Skip to content

Preserve Platform write targets during inlining - #1890

Closed
OskarEichler wants to merge 2 commits into
react:mainfrom
OskarEichler:codex/preserve-platform-write-targets
Closed

OskarEichler wants to merge 2 commits into
react:mainfrom
OskarEichler:codex/preserve-platform-write-targets

Conversation

@OskarEichler

Copy link
Copy Markdown
Contributor

Summary

Metro replaces Platform.OS with a string literal unless it is the direct left side of an assignment. Update/delete expressions, destructuring assignment targets, and direct or nested for...in / for...of targets can therefore be changed semantically or make Babel abort with an invalid literal assignment target.

Walk the member expression's ancestors and preserve it whenever the containing expression is the left side of an assignment or loop, or the argument of an update/delete operation. The React Native Babel preset has a parallel transform; companion React Native PR #58252 applies the same guard there.

Changelog:

 - **[Fix]**: Preserve `Platform.OS` write targets during constant inlining

Test plan

  • Added one combined regression for update/delete, array/object destructuring assignments, and direct/nested for...in and for...of targets.
  • Exact baseline aborts on the direct for...of target; the fixed suite preserves all eight forms.
  • All metro-transform-plugins suites pass: 6/6 suites, 160/160 tests, 41 snapshots.
  • Fresh full Flow check reports 0 errors.
  • Full monorepo build passes for all 17 packages.
  • Targeted ESLint, Prettier, and git diff --check pass.

No public API or intended read-transform behavior changes.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 30, 2026
@facebook-github-tools facebook-github-tools Bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Aug 31, 2026
@OskarEichler

Copy link
Copy Markdown
Contributor Author

A second pass found that the original ancestor walk classified every expression nested anywhere under an assignment LHS as a write. That incorrectly prevented inlining reads such as target[Platform.OS] = value and computed keys in destructuring targets. The walk now continues only through actual assignment-pattern containers and stops at ordinary expressions. Added regressions cover direct computed members, nested array targets, and computed object keys. The focused Jest suite passes (61 tests), and targeted ESLint/Prettier checks pass.

@meta-codesync

meta-codesync Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

@vzaidman has imported this pull request. If you are a Meta employee, you can view this in D118440964.

@meta-codesync meta-codesync Bot closed this in b15a67b Sep 4, 2026
@meta-codesync meta-codesync Bot added the Merged label Sep 4, 2026
@meta-codesync

meta-codesync Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

@vzaidman merged this pull request in b15a67b.

meta-codesync Bot pushed a commit that referenced this pull request Sep 11, 2026
Summary:
Publish Metro 0.87.1 from `main`.

Changelog: [Internal]

Pull Request resolved: #1926

Test Plan:
```
node scripts/updateVersion.js 0.87.1
sl diff --stat
```

Only the 17 `packages/*/package.json` manifests change, and every `0.87.0` reference is replaced.

Draft release notes (from the 91 commits in v0.87.0...main - reverted and internal-only changes omitted):

```
 - **[Feature]**: Add `resolver.schemeResolvers` to resolve URI-scheme-prefixed specifiers (eg `foo:bar`) with custom resolvers (#1804 by robhogan)
 - **[Feature]**: Resolve `metro:babel-runtime/<path>` imports to Metro's own `babel/runtime` dependency (8388f71 by robhogan)
 - **[Feature]**: `metro-file-map`: Add `crawlerFactory` to optionally replace the built-in Watchman / Node crawlers (68022f0 by vzaidman)
 - **[Feature]**: `metro-babel-transformer`: Pass `inlinePlatform` to Babel presets via `caller` (6bbe095 by robhogan)
 - **[Fix]**: Replace `image-size` dependency with vendored parsers, fix CVE alerts (#1860 by robhogan)
 - **[Fix]**: Include `charset=utf-8` in the `Content-Type` of text assets and source files served by the dev server (#1888 by robhogan)
 - **[Fix]**: Preserve `Platform.OS` write targets during constant inlining (#1890 by OskarEichler)
 - **[Fix]**: Inline the last duplicate key from static `Platform.select` object literals (#1889 by OskarEichler)
 - **[Fix]**: `FallbackWatcher` no longer misses files written to a directory while it is being crawled (#1907 by robhogan)
 - **[Fix]**: `HttpStore`: Handle socket errors during writes, so they're retried rather than crashing the process (fad3b88)
 - **[Fix]**: Treat `CI=false` and `CI=0` as not-CI when defaulting `watch`. CI is now detected from `process.env.CI` only, dropping the `ci-info` dependency (61e4592 by robhogan)
 - **[Fix]**: Fix Fast Refresh hitting undefined modules when using lazy (segmented) bundles in dev (c70d0ae by robhogan)
 - **[Performance]**: `FallbackWatcher`: Drop `walker` dependency, reduce crawl RSS by ~34% and peak heap by ~41% (#1906 by robhogan)
 - **[Performance]**: Don't emit redundant empty dependency map arrays for modules with no dependencies (#1858 by robhogan)
 - **[Types]**: Restore publishing of private (underscore-prefixed) fields in TypeScript declarations (#1876 by robhogan)
 - **[Types]**: Fix async functions being inferred as returning `void` (not `Promise<void>`), eg `MetroServer#end` (#1909 by robhogan)
 - **[Types]**: Tidy up generated types, allow nullable props to be omitted in more places (#1885 by robhogan)

> NOTE: Experimental features are not covered by semver and can change at any time.

 - **[Experimental]**: Add `serializer.unstable_inlineDependencyMap` to inline module IDs at serialisation time (#1786 by robhogan)
 - **[Experimental]**: Add `serializer.unstable_getAsyncDependencyPath` to supply custom `paths` to framework-defined `__loadBundleAsync` (#1855 by robhogan)
 - **[Experimental]**: Remove `transformer.unstable_renameRequire` - `require` is never renamed (42577f7 by robhogan)
 - **[Experimental]**: `experimentalImportSupport`: Fix `export * from` re-exporting the source module's default export (#1777 by robhogan)

 **Full Changelog**: v0.87.0...v0.87.1
```

Reviewed By: zeyap

Differential Revision: D119675927

Pulled By: GijsWeterings

fbshipit-source-id: daf17f3501b4725c21c39b4dfd5c82b95e01dca5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant