experimentalImportSupport: Fix export * from incorrectly re-exporting the default export#1777
Open
robhogan wants to merge 1 commit into
Open
experimentalImportSupport: Fix export * from incorrectly re-exporting the default export#1777robhogan wants to merge 1 commit into
export * from incorrectly re-exporting the default export#1777robhogan wants to merge 1 commit into
Conversation
…e default export Summary: Fix a spec conformance bug in `export * from 'foo'` under `experimentalImportSupport` (`import-export-plugin`). Currently, Metro re-exports all exports including the source module's default export. ## Spec https://tc39.es/ecma262/multipage/ecmascript-language-scripts-and-modules.html#sec-getexportednames `8.d.i` ``` 8. For each ExportEntry Record exportEntry of module.[[StarExportEntries]], do a. Assert: exportEntry.[[ModuleRequest]] is not null. b. Let requestedModule be GetImportedModule(module, exportEntry.[[ModuleRequest]]). c. Let starNames be requestedModule.GetExportedNames(exportStarSet). d. For each element name of starNames, do -> i. If name is not "default", then 1. If exportedNames does not contain name, then a. Append name to exportedNames. ``` Node.js, Babel's `transform-modules-commonjs`, and browsers all conform to this spec. ## Fix Trivially `continue` through a key named "default" in our `exportAllTemplate`. NOTE: `export * as ns from 'foo'` is unaffected: it goes through the `ExportNamespaceSpecifier` / `importAll` path, and per spec the namespace object still exposes the source's default. ## Possible alternatives + benchmarks Compiled release HBC under SH trunk (`hermesc -O -emit-binary`) and run in the `hermes` VM. - One "op" = one full re-export of a module with 20 named exports + `default` (which every form must skip). - Throughput = median of 9 runs × 250k ops. - Bytecode = marginal HBC size of the isolated `f(exports, REQUIRED)` body vs an empty-body baseline. | Form | Input | Throughput (ops/s) | stddev | rel | HBC bytes (Δ) | |---|---|--:|--:|--:|--:| | `for-in`, no `default` guard (re-exports default) | 20 + `default` | 99,602 | 1,041 | 0.95× | +96 | | `for-in` + `if (KEY === "default") continue;` (current) | 20 + `default` | 104,778 | 1,025 | 1.00× | +126 | | `for-in` + `if (KEY !== "default") { ... }` | 20 + `default` | 104,515 | 879 | 1.00× | +127 | | object spread + `delete default` | 20 + `default` | 91,709 | 583 | 0.88× | +148 | | `Object.keys` + indexed for-loop | 20 + `default` | 90,351 | 672 | 0.86× | +206 | | `Object.getOwnPropertyNames` + for-loop | 20 + `default` | 89,831 | 680 | 0.86× | +224 | | `for-in` + `hasOwnProperty` guard | 20 + `default` | 78,939 | 862 | 0.75× | +239 | | `Object.keys().forEach` | 20 + `default` | 73,746 | 346 | 0.70× | +273 | | `Object.assign` + `delete default` | 20 + `default` | 61,395 | 567 | 0.59× | +139 | | `for-in` + `default` guard (guard is pure overhead) | 20, **no** `default` | 104,080 | 538 | 0.99× | +126 | | `for-in`, no guard | 20, **no** `default` | 104,866 | 634 | 1.00× | +96 | **Conclusion:** both `for-in` guard styles are fastest and smallest of all semantically-equivalent forms (differing by ~3% throughput / 1 byte — within noise). - The `default` guard's runtime effect flips with the input: - **default present:** guard is **+5.2%** faster (104,778 vs 99,602) — skipping the `default` property write outweighs the per-key comparisons. - **default absent:** guard is **−0.7%** slower (104,080 vs 104,866) — pure overhead, nothing to skip. - The guard is order of magnitudes cheaper than prop assignment, adding it has negligible effect. - Adding the guard adds 5 bytes of HBC per occurrence (plus one-time 16 bytes for the "default" string table entry - which is there in a real app anyway). ## Changelog ``` - **[Experimental]** experimentalImportSupport: Fix `export * from` incorrectly re-exporting default exports ``` Differential Revision: D111910097
Contributor
|
@robhogan has exported this pull request. If you are a Meta employee, you can view the originating Diff in D111910097. |
export * incorrectly re-exporting the default exportexport * from incorrectly re-exporting the default export
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
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:
Fix a spec conformance bug in
export * from 'foo'underexperimentalImportSupport(import-export-plugin).Currently, Metro re-exports all exports including the source module's default export.
Spec
https://tc39.es/ecma262/multipage/ecmascript-language-scripts-and-modules.html#sec-getexportednames
8.d.iNode.js, Babel's
transform-modules-commonjs, and browsers all conform to this spec.Fix
Trivially
continuethrough a key named "default" in ourexportAllTemplate.NOTE:
export * as ns from 'foo'is unaffected: it goes through theExportNamespaceSpecifier/importAllpath, and per spec the namespace object still exposes the source's default.Possible alternatives + benchmarks
Compiled release HBC under SH trunk (
hermesc -O -emit-binary) and run in thehermesVM.default(which every form must skip).f(exports, REQUIRED)body vs an empty-body baseline.for-in, nodefaultguard (re-exports default)defaultfor-in+if (KEY === "default") continue;(current)defaultfor-in+if (KEY !== "default") { ... }defaultdelete defaultdefaultObject.keys+ indexed for-loopdefaultObject.getOwnPropertyNames+ for-loopdefaultfor-in+hasOwnPropertyguarddefaultObject.keys().forEachdefaultObject.assign+delete defaultdefaultfor-in+defaultguard (guard is pure overhead)defaultfor-in, no guarddefaultConclusion: both
for-inguard styles are fastest and smallest of allsemantically-equivalent forms (differing by ~3% throughput / 1 byte — within
noise).
defaultguard's runtime effect flips with the input:defaultproperty write outweighs the per-key comparisons.nothing to skip.
Changelog
Differential Revision: D111910097