[Hotfix] 0.76.9 #1169
Merged
[Hotfix] 0.76.9 #1169
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: Pull Request resolved: #1083 Fixes a crash where a race in overlapping file change processing could occasionally throw "unexpected null". Fixes #1015 ## Root cause The problem logic is in some instrumentation code introduced in D40346848. `metro-file-map` batches changes using `setInterval(emitChange, 30)`. The instrumentation attempted to record the time of the first event in a given batch, and then "reset the clock" when the batch is emitted to the consumer. The problem occurred when an emit interval fell such that a non-empty `emitChange` occurred *during* the async `_processFile` step of a subsequent change. The first emit would reset the "start time" to `null`, and the second `emitChange` would fail at `nullthrows(eventStartTimestamp)`, because `eventStartTimestamp` is only set *before* `_processFile` starts (and only if already `null`). The (over)writing of `eventStartTimestamp` was flawed, because we don't know at the start of change processing that this event is going to be the start of a batch - this only becomes clear when an event is subsequently enqueued for emit after file processing. ## This diff This changes the recording of the start time such that we take a timestamp on every change, and record it as the batch start time on the creation of a new batch. We rearrange the various moving parts into a nullable object, so that "first event time" is tightly coupled to the batch it describes, and can never be `null` for a non-empty batch. ## Changelog ``` * **[Fix]:** Fix "unexpected null" crash when handling a batch of file changes. ``` Reviewed By: huntie Differential Revision: D49272782 fbshipit-source-id: 8e1a4ebb6876fad982af3aa8a1922c6f14236040
facebook-github-bot
added
the
CLA Signed
Summary: Metro's core `Graph` maintains a `CountingSet` of inverse "eager" dependencies for each module - zero inverse dependencies triggers a module's removal from the graph. Currently, this is not decremented with on dependency removals consistently with the way it is incremented. In particular, removing an async dependency (in combination with `options.lazy`) removes an inverse dependency, even though the reverse does not add one. This leads to incorrect deletion of module B from the graph when module A has both sync and async dependencies on B, and the async dependency is removed. ``` * **[Fix]**: Fix Fast Refresh edge case where removing an async dependency could incorrectly remove modules from the graph. ``` Reviewed By: motiz88 Differential Revision: D50309038 fbshipit-source-id: 4a44703ac31fc400736c5666a51ca26783a34d19
Summary: **Summary** The sourceMapUrl in bundles built for windows/macOS is of the format ``` //# sourceMappingURL=//localhost:8081/index.map?platform=windows&dev=true&hot=false&inlineSourceMap=false ``` This causes the chrome debugger to be unable to correctly load the source map. It turns out there is already code to modify this specifically for android/iOS. This change makes windows/macOS behavior match android/iOS. See microsoft/react-native-windows#9407. Pull Request resolved: #763 Reviewed By: robhogan Differential Revision: D51031030 Pulled By: motiz88 fbshipit-source-id: a325e417d725c53c5d96866d80f300a72389e4c2
…ame` (#1136) Summary: This fixes #1128 by calling the `resolvePackage` instead of `resolveModulePath` in `resolveHasteName`. Only `resolvePackage` has the code to resolve package "exports" and it calls `resolveModulePath` as a fallback. Changelog: [Experimental] When enabled, the `"exports"` field is now considered for Haste packages (which could include local monorepo packages) Pull Request resolved: #1136 Test Plan: I've added a failing test which passed as the fix got implemented. Reviewed By: huntie Differential Revision: D51346769 Pulled By: robhogan fbshipit-source-id: 8a003d5b147b73d344365db7cff8187ff946013d
Summary:
We use unstable_serverRoot heavily in Expo to support monorepos and consistently have issues where saving a file will log an error in the console:
```
Error: Unable to resolve module ./app/index from /Users/evanbacon/Documents/GitHub/expo/.:
None of these files exist:
* ../../app/index(.web.ts|.ts|.web.tsx|.tsx|.web.mjs|.mjs|.web.js|.js|.web.jsx|.jsx|.web.json|.json|.web.cjs|.cjs|.web.scss|.scss|.web.sass|.sass|.web.css|.css|.web.cjs|.cjs)
* ../../app/index/index(.web.ts|.ts|.web.tsx|.tsx|.web.mjs|.mjs|.web.js|.js|.web.jsx|.jsx|.web.json|.json|.web.cjs|.cjs|.web.scss|.scss|.web.sass|.sass|.web.css|.css|.web.cjs|.cjs)
at ModuleResolver.resolveDependency (/Users/evanbacon/Documents/GitHub/expo/node_modules/metro/src/node-haste/DependencyGraph/ModuleResolution.js:114:15)
at DependencyGraph.resolveDependency (/Users/evanbacon/Documents/GitHub/expo/node_modules/metro/src/node-haste/DependencyGraph.js:277:43)
at /Users/evanbacon/Documents/GitHub/expo/node_modules/metro/src/lib/transformHelpers.js:169:21
at Server._resolveRelativePath (/Users/evanbacon/Documents/GitHub/expo/node_modules/metro/src/Server.js:1045:12)
at Server.requestProcessor [as _processSourceMapRequest] (/Users/evanbacon/Documents/GitHub/expo/node_modules/metro/src/Server.js:449:37)
at Server._processRequest (/Users/evanbacon/Documents/GitHub/expo/node_modules/metro/src/Server.js:396:7)
```
This is because the HMR `sourceMappingURL` is being calculated relative to the projectRoot instead of the serverRoot, but all files are being resolved relative to the serverRoot.
Pull Request resolved: #1137
Test Plan:
[rob]
Before:
{F1154753453}
After:
{F1154752435}
Reviewed By: huntie
Differential Revision: D51467903
Pulled By: robhogan
fbshipit-source-id: a14ca98a9d95eb18d521b11592b2d251fc09f178
robhogan
force-pushed
the
hotfix/0.76.9
branch
from
2e1377b
to
f9d4bb5
Compare
… paths Summary: Issues such as #1017 (and similar previously) have had users running into broken setups due to "multiple versions of Metro" installed. Ideally, multiple Metros wouldn't be a problem. Metro packages already specify explicit, pinned dependencies on each other, and well-behaved package resolutions should stay within those disconnected graphs of Metro versions. In particular `metro` already specifies its dependency on `metro-transform-worker`, so it wasn't obvious how we were "escaping" from the correct `metro` and ending up inside the wrong `metro-transform-worker` here. I was able to reproduce #1017 with Berry and debug it. It turns out that the problem is the use of the (undocumented) `transformer.workerPath` option, which defaults to `'metro/src/DeltaBundler/Worker'`. We pass that to `jest-worker`, which forks a child process `jest-worker/build/workers/processChild`, which resolves the given path *relative to `jest-worker`* - so usually resolves the *hoisted* `metro`, whatever that happens to be. This fixes the issue by resolving the path from the parent process, within `metro`, and passing an absolute path to `jest-worker`. Changelog: ``` - **[Fix]**: Incorrect worker resolution when multiple `metro` versions are installed. ``` Reviewed By: huntie Differential Revision: D47209874 fbshipit-source-id: 5aa101852e9d33af6f41c118252d9874701a01be
…ional chain (?.) are skipped (#1178) Summary: Fixes: #1167 `constant-folding-plugin` replaces this call `void foo?.();` with `undefined;`. This is because the `evaluate()` function marks the optional call expressions as safe, making the `UnaryExpression` visitor think it is safe to replace it with the `value` returned by the `evaluate()`, in this case `undefined`. This PR makes the `evaluate()` function mark the optional call expressions as unsafe. ``` * **[Fix]**: `constant-folding-plugin`: Don't fold optional function calls (`foo.?()`). ``` Pull Request resolved: #1178 Test Plan: I have added 2 tests to cover these changes: - does not transform optional chained call into `undefined` - does not transform `void` prefixed optional chained call into `undefined` Reviewed By: GijsWeterings Differential Revision: D52780448 Pulled By: robhogan fbshipit-source-id: c84288adc1fec6c2e875fd766c5a6b0997a36c62
…#1195) Summary: We've encountered this as part of a report on the `expo` repo: expo/expo#25965 There, an issue is caused by a dependency using an export alias named `__DEV__`, which the `inlinePlugin` is trying to replace: ```js export { DEV as __DEV__ }; ``` This only occurs in certain cases, since usually, as part of the iOS/Android builds, we would be applying the `inlinePlugin` only after export statements, where this issue may occur, have been transpiled away. However, looking at this issue, I found more cases that would fail with the current implementation of the `inlinePlugin`. This is part of what I added in tests but not an exhaustive list: - Shorthand object methods (`{ __DEV__() {} }`) - Labelled statements (`__DEV__: {};`) - Class methods (`class { __DEV__() {} }`) - Optional property member access (`x?.__DEV__`) All of these issues can be addressed by letting this replacement use `ReferencedIdentifier` as a visitor, rather than just `Identifier`. Pull Request resolved: #1195 Test Plan: - Tests have been added to `inline-plugin-test.js` to reflect the mentioned cases. Reviewed By: huntie Differential Revision: D52909519 Pulled By: motiz88 fbshipit-source-id: 37a6459bc917701fe8474c33ccae41cb484e92f0
…x pnpm (etc) resolution (#1199) Summary: Pull Request resolved: #1199 Related: #1172 This is the minimally invasive change to fix resolution of the default `minifierPath: 'metro-minify-terser'`, especially under isolated node_modules layouts. `minifierPath` is required/resolved only from `metro-transform-worker`: - https://github.com/facebook/metro/blob/v0.80.4/packages/metro-transform-worker/src/utils/getMinifier.js#L22 - https://github.com/facebook/metro/blob/v0.80.4/packages/metro-transform-worker/src/index.js#L656 Per the [current docs for `minifierPath`](https://metrobundler.dev/docs/configuration/#minifierpath), a module specifier relative to `metro-transform-worker` is explicitly acceptable: > Type: string (default: 'metro-minify-terser') > Path, or package name resolvable from metro-transform-worker, to the minifier that minifies the code after transformation. Unlike #1172 (thanks tido64 for flagging), this doesn't modify the defaults and can be released in a patch release. The approach in that PR (using fully resolved paths in config) may be the better long-term fix though, so this patch shouldn't be regarded as superseding it. Changelog: ``` - **[Fix]:** Move `metro-minify-terser` dependency to fix resolution under isolated node_modules (pnpm, etc). ``` Reviewed By: huntie Differential Revision: D53000650 fbshipit-source-id: 251f52c17af58c88ebedb387ac92ecbe788772ea
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## 0.76.x #1169 +/- ##
=======================================
Coverage 80.43% 80.44%
=======================================
Files 216 216
Lines 11088 11096 +8
Branches 2748 2753 +5
=======================================
+ Hits 8919 8926 +7
- Misses 2169 2170 +1 ☔ View full report in Codecov by Sentry. |
huntie
approved these changes
Jan 29, 2024
… allow prerelease tags (#1086) Summary: Currently, when we `npm publish --workspaces` on tag creation, NPM implicitly tags with "latest". This is fine in the regular release flow, but when we release a hotfix (a patch release on an old minor, from a release branch) the tag is incorrect. This causes issues that some folks notice within minutes: #931, #1057. Currently, it has to be manually corrected using `dist-tag` on each package after publishing. This adds some logic in CircleCI to automatically tag with `latest` explicitly *only if* the tag is present on `main`, and not on any release branch (of the form `0.79.x`). OTOH, if the release looks like a hotfix (present on a release branch but *not* on `main`) we tag with the release branch name. There is no way in NPM not to have any dist-tag, so specifying *something* is the only way to prevent it assuming `latest`. If the release is ambiguous (edge case: we go back and tag a common ancestor of `main` and `0.123.x` with `v0.123.0-alpha`) the publish aborts. Pull Request resolved: #1086 Test Plan: - Tested (after many iterations!) at my fork https://github.com/robhogan/metro, eg https://app.circleci.com/pipelines/github/robhogan/metro/74/workflows/5a375202-5863-43fd-b633-4a983b0ed4b8/jobs/264 - I'm about to run a hotfix release to pull some bug fixes back for RN 0.72, so that'll be the first "live" test. Reviewed By: huntie Differential Revision: D49461403 Pulled By: robhogan fbshipit-source-id: 26fd37d90ccae9d2125e9ef1cc8ba538ee6beb85
robhogan
force-pushed
the
hotfix/0.76.9
branch
from
675dc01
to
cff071d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Changelog:
metro-minify-terserdependency to fix resolution under isolated node_modules (pnpm, etc).(Movemetro-minify-terserdependency tometro-transform-worker, fix pnpm (etc) resolution #1199 by @robhogan)inline-plugin: Don't replace__DEV__in invalid Babel AST locations. (FixinlinePluginreplacing__DEV__in invalid Babel AST locations #1195 by @kitten)constant-folding-plugin: Don't fold optional function calls (foo.?()). (fix(#1167): function calls prefixed withvoidtogether with the optional chain (?.) are skipped #1178 by @Gamote)metroversions are installed. (6d46078 by @robhogan)"exports"field is now considered for Haste packages (which could include local monorepo packages) (UseresolvePackageinstead ofresolveModulePathinresolveHasteName#1136 by @kraenhansen)sourceMappingUrlin HMR payloads when usingunstable_serverRoot(fix: support serverRoot with hmr serializer chunks #1137 by @EvanBacon)Test plan
These were all cherry picked from
mainwith minimal intervention so should be safe, but I'll do some manual smoke testing post-publish before bumping RN.