Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Hotfix] 0.76.9 #1169

Merged
merged 11 commits into from
Jan 30, 2024
Merged

[Hotfix] 0.76.9 #1169

merged 11 commits into from
Jan 30, 2024

Commits on Jan 9, 2024

  1. Fix "unexpected null" handling overlapping file changes (#1083)

    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
    robhogan committed Jan 9, 2024
    Configuration menu
    Copy the full SHA
    4cc6b08 View commit details
    Browse the repository at this point in the history
  2. Fix - removing an async dependency may incorrectly free its target

    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
    robhogan committed Jan 9, 2024
    Configuration menu
    Copy the full SHA
    0476bb2 View commit details
    Browse the repository at this point in the history
  3. Fix sourceMapURL when building bundles for windows/macOS (#763)

    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
    acoates-ms authored and robhogan committed Jan 9, 2024
    Configuration menu
    Copy the full SHA
    6100e4a View commit details
    Browse the repository at this point in the history
  4. Use resolvePackage instead of resolveModulePath in `resolveHasteN…

    …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
    kraenhansen authored and robhogan committed Jan 9, 2024
    Configuration menu
    Copy the full SHA
    4172b5a View commit details
    Browse the repository at this point in the history
  5. fix: support serverRoot with hmr serializer chunks (#1137)

    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
    EvanBacon authored and robhogan committed Jan 9, 2024
    Configuration menu
    Copy the full SHA
    f9d4bb5 View commit details
    Browse the repository at this point in the history

Commits on Jan 29, 2024

  1. Fix mismatched metro-transform-worker version - use absolute worker…

    … 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
    robhogan committed Jan 29, 2024
    Configuration menu
    Copy the full SHA
    7c4de9a View commit details
    Browse the repository at this point in the history
  2. fix(#1167): function calls prefixed with void together with the opt…

    …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
    Gamote authored and robhogan committed Jan 29, 2024
    Configuration menu
    Copy the full SHA
    265fef1 View commit details
    Browse the repository at this point in the history
  3. Fix inlinePlugin replacing __DEV__ in invalid Babel AST locations (

    …#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
    kitten authored and robhogan committed Jan 29, 2024
    Configuration menu
    Copy the full SHA
    fce4c52 View commit details
    Browse the repository at this point in the history
  4. Move metro-minify-terser dependency to metro-transform-worker, fi…

    …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
    robhogan committed Jan 29, 2024
    Configuration menu
    Copy the full SHA
    2526739 View commit details
    Browse the repository at this point in the history

Commits on Jan 30, 2024

  1. CircleCI: Don't tag hotfix releases with latest during publish job,…

    … 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 committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    0feb8b4 View commit details
    Browse the repository at this point in the history
  2. Publish 0.76.9

    robhogan committed Jan 30, 2024
    Configuration menu
    Copy the full SHA
    cff071d View commit details
    Browse the repository at this point in the history