-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Support ES6 module syntax in Acorn, Babel and Closure #23730
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
base: main
Are you sure you want to change the base?
Changes from 26 commits
ea0ed86
decb8f8
908c47c
985f4dc
254ed04
e194143
047b852
b077e9c
b7c92f5
1fbc5ca
dbf6e5e
e9f3e23
d745291
31cf27b
f2fbf66
6ac9b01
083405a
ef36474
df79ebe
711efc1
0972e63
5fd7830
06d1884
0ea5e1c
bd18866
2d6ba19
af43dd1
5c2f12b
0b218f2
cf6417c
8283862
f74a9b7
11a3324
decf760
f8be17f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -112,8 +112,7 @@ if (ENVIRONMENT_IS_NODE) { | |
| // When building an ES module `require` is not normally available. | ||
| // We need to use `createRequire()` to construct the require()` function. | ||
| const { createRequire } = await import('node:module'); | ||
| /** @suppress{duplicate} */ | ||
| var require = createRequire(import.meta.url); | ||
| global.require = createRequire(import.meta.url); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we prefer
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We use (there was a reason I originally did this, likely something related to Closure, but I can't recall the exact reason anymore)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ... looking at commit ef36474, I think that I know the reason behind this. This was likely necessary for the now-removed
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reverted this with commit af43dd1, which further reduces the codesize expectations of |
||
| #endif | ||
|
|
||
| #if PTHREADS || WASM_WORKERS | ||
|
|
@@ -130,7 +129,7 @@ if (ENVIRONMENT_IS_NODE) { | |
| #endif | ||
| #endif // PTHREADS || WASM_WORKERS | ||
| } | ||
| #endif // ENVIRONMENT_MAY_BE_NODE | ||
| #endif // ENVIRONMENT_MAY_BE_NODE && (EXPORT_ES6 || PTHREADS || WASM_WORKERS) | ||
|
|
||
| // --pre-jses are emitted after the Module integration code, so that they can | ||
| // refer to Module (if they choose; they can also define Module) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,7 @@ version published in npm. | |
| This `terser.js` bundle in this directory was built from our fork of terser | ||
| which lives at: https://github.com/emscripten-core/terser/ | ||
|
|
||
| The current patches are stored in the `emscripten_patches_v4.8.0` branch. | ||
| The current patches are stored in the `emscripten_patches_v5.18.2` branch. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a driveby fix? Was this just no updated at some point?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, a drive-by fix, since we're now using Terser 5.18.2. |
||
|
|
||
| To make changes to this code please submit patches to | ||
| https://github.com/emscripten-core/terser/ and then re-create this bundle | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -427,6 +427,11 @@ def emscript(in_wasm, out_wasm, outfile_js, js_syms, finalize=True, base_metadat | |||||||||||||||||||||||||||||||
| assert '//FORWARDED_DATA:' in out, 'Did not receive forwarded data in pre output - process failed?' | ||||||||||||||||||||||||||||||||
| glue, forwarded_data = out.split('//FORWARDED_DATA:', 1) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| # Unmangle previously mangled `import.meta` references in lib*.js. | ||||||||||||||||||||||||||||||||
| # See also: `LibraryManager.load` in modules.mjs. | ||||||||||||||||||||||||||||||||
| if settings.EXPORT_ES6: | ||||||||||||||||||||||||||||||||
| glue = glue.replace('EMSCRIPTEN$IMPORT$META', 'import.meta') | ||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason we cannot do the demangling in the JS compiler (i.e. the same place where we do the mangling?)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, good idea. Let me try to move this to the JS compiler, I think that would work.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ... that didn't work because the library contents (e.g. Lines 394 to 401 in 426fb2d
So, this is probably the only place where the full file contents is available and where you can a single replace in one go. Added a comment with cf6417c instead.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we could run this transform over all of the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Commit 11a3324 moves this to Lines 888 to 892 in d984d06
However, Lines 882 to 883 in d984d06
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| forwarded_json = json.loads(forwarded_data) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| if forwarded_json['warnings']: | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need to move from
preprocessto here?Its a little confusing to have this mangling done in two different places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved back to
preprocesswith commit 5c2f12b (and also updated to usereplaceAllin commit 0b218f2 to avoid regex).