ESM live bindings: rewrite the experiment runbook against the current stack - #1865
Open
vzaidman wants to merge 2 commits into
Open
ESM live bindings: rewrite the experiment runbook against the current stack#1865vzaidman wants to merge 2 commits into
vzaidman wants to merge 2 commits into
Conversation
…ults (#1862) Summary: ## What Two pieces of infrastructure that later diffs in the stack build on: AST-based ESM classification in `esmClassification.js`, and a tri-state `isESModule` signal on `JsOutput.data`. The module exports a positive check and a negative one. They are not complements, and it takes both to say a module has no ESM interop. ## `definesESModuleInterop` - the positive check Detects a truthy top-level `exports.__esModule` or `module.exports.__esModule`, where truthy means `true`, `1` or `!0`. Four shapes, covering the common ESM->CJS toolchains: ```js Object.defineProperty(exports, '__esModule', {value: true}); // metro import-export-plugin, babel, tsc, rollup Object.defineProperty(module.exports, '__esModule', {value: true}); // hand-written interop wrappers exports.__esModule = true; // babel loose, older tsc, rollup if-default-prop module.exports = fn, module.exports.__esModule = true, ... // babel/runtime/helpers (sequence expression) ``` AST-based rather than a scan of generated source, so it is robust to whitespace, quoting and property-attribute order. ## `canDefineESModuleInterop` - the negative check The positive check only inspects top-level statements, so a false result means "no marker here", not "no marker". The marker can be installed from anywhere the exports object is reachable: a self-contained bundle can hand its exports to a helper that sets the key (webpack's `__webpack_require__.r`), needing no dependencies and staying invisible to a statement scan. `canDefineESModuleInterop` closes that gap. It returns false - nothing in the module can produce the key - only when all three hold: 1. `__esModule` occurs nowhere, as an identifier or as a string. 2. `exports`/`module` are only ever read as the target of an export write we can enumerate. Never aliased, passed to a function, or accessed with a computed key. 3. No construct can define a property under a key that is not literally in the source: computed member assignment, computed object key, spread, `Object.assign`, `Object.defineProperties`. (1) bars the literal, (3) bars constructing it dynamically, and (2) stops the exports object escaping somewhere the other two cannot see. Anything unrecognised returns true, so the assertion is only ever made on modules whose exports are fully accounted for. This is what lets a `false` be an assertion about the module rather than a statement about where we happened to look. ## Tri-state signal The plugin hint is positive-only: `import-export-plugin` sets `out.isESModule = true` when it processes ESM syntax and leaves the field unset otherwise. A definite `false` there would suppress the AST fallback for ESM already lowered to CJS upstream by babel/tsc/rollup. The worker combines hint and fallback, then widens the result to a tri-state on `JsOutput.data.isESModule`: ```js const isESModule = importExportOut.isESModule ?? definesESModuleInterop(ast); const hasNoESModuleInterop = !isESModule && dependencies.length === 0 && !canDefineESModuleInterop(ast); ``` - `true`: definitely ESM, so it has a real `.default`. - `false`: no ESM interop. No marker, no dependencies, and no expression that could define the marker out of view. This establishes the absence of ESM interop, not the presence of CommonJS - a script or an empty module qualifies too. Common at FBiOS scale via generated Relay fragments. - unset: undetermined. A module with `require(...)` calls but no marker can still expose interop via `module.exports = require('./esm')`, so consumers must fall back to the runtime helper. The dependency clause is kept alongside the new check because re-exporting an ES module wholesale is a property of the graph, not of this module's syntax, so it is not something an AST check can rule out. Detection lives in the worker rather than `collectDependencies` because the worker owns both the plugin hint and the JsOutput. `getSourceMapInfo` stops spreading `JsOutput.data` and names its four fields explicitly, so the new optional field cannot leak into source map info: ```js const data = getJsOutput(module).data; return {code: data.code, functionMap: data.functionMap, lineCount: data.lineCount, map: data.map, ...}; ``` OSS serialisers pick the field up automatically via `module.output[].data`. Metro-Buck bridges it explicitly in a follow-up, since it re-serialises through its own module IR. ## Coverage Against the release FBiOS RN bundle (17,058 modules): 12,475 (73.1%) detected ESM, 4,581 (26.8%) with no `__esModule` at runtime, 2 (0.01%) missed. Both misses are IIFE-wrapped UMD bundles (one of them `whatwg-fetch.umd.js`) with the marker inside an inner factory; resolving `exports` through the IIFE is not worth it for two modules. Those numbers measure the positive check, which is unchanged. The negative check does not affect them - it only narrows which of the unmarked modules are asserted `false` rather than left unset. UMD of exactly that shape is now declined rather than asserted, so the two misses cannot become a wrong `false` if such a module has no dependencies. Reviewed By: huntie Differential Revision: D111629268
… stack Summary: Replaces the stale runbook from the superseded stack (D111149711, which described D111148330's shape) with one written against the diffs that are actually going to land, and documents two things we didn't know when it was first written. What changed vs the old runbook: - **Stack map is current.** D111629268 → D111629272 → D111629271 → D111529091, with the Hermes prototype (D111629267) explicitly parked as a side branch and the superseded diffs listed as do-not-land. - **New section: how the two halves compose.** `require(dep).default` is a fresh property read on every evaluation, so for ESM-detected dependencies the serialize-time rewrite is *inherently live* **and** `CallRequire`-eligible — it supersedes the live `metroImportDefault` helper for exactly those sites, while CJS/unknown dependencies still rely on the live helper from D111529091. Both paths are needed; the size/TTI numbers must be taken on the combined stack. - **New section: ordering / caching.** Records why `inline-requires` (transform time) cannot defeat the rewrite (serialize time), and that `unstable_liveBindings` is in the transform cache key and graph id, so control and treatment artifacts can't be confused. - **New: dev bundles are not rewritten.** `keepRequireNames: options.dev` makes `collectDependencies` emit `_$$_IMPORT_DEFAULT(depMap[k], './x')`, and the rewrite deliberately matches only the single-argument form. This is fine (the rewrite targets optimized bundles) but it means the rewrite cannot be validated in a dev bundle — worth knowing before someone tries. - **New: the transform is not free when the flag is OFF.** The live `metroImportDefault` branch and its prelude-global read ship in `metro-runtime`'s `require.js` polyfill for every bundle regardless of the flag: ~+320 bytes JS on the metro-buck e2e fixture. The old runbook's "zero behaviour change when disabled" framing was wrong. - **Experiment plan is executable.** Explicitly turns `unstable_staticHermesOptimizedRequire` on in *both* arms so the only delta is `unstable_liveBindings`, and adds a step to verify the rewrite actually fired (helper count vs `).default` count) before trusting any measurement. - **Follow-ups list pruned** of items already addressed by D111529091's getter-based re-export forwarding and by D111568389. Docs only, no code. Differential Revision: D114885628
meta-cla
Bot
added
the
CLA Signed
Contributor
|
@vzaidman has exported this pull request. If you are a Meta employee, you can view the originating Diff in D114885628. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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:
Replaces the stale runbook from the superseded stack (D111149711, which described
D111148330's shape) with one written against the diffs that are actually going to
land, and documents two things we didn't know when it was first written.
What changed vs the old runbook:
with the Hermes prototype (D111629267) explicitly parked as a side branch and
the superseded diffs listed as do-not-land.
require(dep).defaultis a freshproperty read on every evaluation, so for ESM-detected dependencies the
serialize-time rewrite is inherently live and
CallRequire-eligible — itsupersedes the live
metroImportDefaulthelper for exactly those sites, whileCJS/unknown dependencies still rely on the live helper from D111529091. Both
paths are needed; the size/TTI numbers must be taken on the combined stack.
inline-requires(transformtime) cannot defeat the rewrite (serialize time), and that
unstable_liveBindingsis in the transform cache key and graph id, so controland treatment artifacts can't be confused.
keepRequireNames: options.devmakescollectDependenciesemit_$$_IMPORT_DEFAULT(depMap[k], './x'), and therewrite deliberately matches only the single-argument form. This is fine (the
rewrite targets optimized bundles) but it means the rewrite cannot be validated
in a dev bundle — worth knowing before someone tries.
metroImportDefaultbranch and its prelude-global read ship inmetro-runtime'srequire.jspolyfill for every bundle regardless of the flag:~+320 bytes JS on the metro-buck e2e fixture. The old runbook's "zero behaviour
change when disabled" framing was wrong.
unstable_staticHermesOptimizedRequireon in both arms so the only delta isunstable_liveBindings, and adds a step to verify the rewrite actually fired(helper count vs
).defaultcount) before trusting any measurement.getter-based re-export forwarding and by D111568389.
Docs only, no code.
Differential Revision: D114885628