Skip to content

module: use cjsCache over esm injection#34605

Closed
guybedford wants to merge 3 commits intonodejs:masterfrom
guybedford:two-phase-esm-injection
Closed

module: use cjsCache over esm injection#34605
guybedford wants to merge 3 commits intonodejs:masterfrom
guybedford:two-phase-esm-injection

Conversation

@guybedford
Copy link
Contributor

@guybedford guybedford commented Aug 3, 2020 •
edited
Loading

This is the replacement for #34467 as an alternative mechanism to ensure that named exports loaders (such as https://github.com/guybedford/cjs-named-exports-loader) are fully possible without issues. Without this PR such loaders cannot work for CJS loaded via require() before it is loaded via import().

As discussed, this PR retains the existing CJS snapshotting behaviour so that the module.exports value is captured for all CommonJS modules and stored in the ESMLoader.cjsCache map. This map is changed to be a weakmap keyed by the module object so that deletions in Module._cache allow this map to be GC'd.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 3, 2020

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/4193

@nodejs-github-bot nodejs-github-bot added the esm Issues and PRs related to the ECMAScript Modules implementation. label Aug 3, 2020
@guybedford
Copy link
Contributor Author

guybedford commented Aug 3, 2020

//cc @nodejs/modules-active-members

@devsnek
Copy link
Member

devsnek commented Aug 3, 2020

You could also do a weakmap from the module instance to the initial module.exports value. That way you support items being deleted from the cache (you're already playing with fire if you're doing that) but most code still gets the benefit of consistent exports.

@guybedford
Copy link
Contributor Author

guybedford commented Aug 3, 2020

That's a great way to work around the GC issue! I've pushed that up in the latest commit, which should avoid the problem entirely, thanks.

aduh95
aduh95 reviewed Aug 8, 2020
View reviewed changes
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 8, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/32685/

@guybedford
Copy link
Contributor Author

guybedford commented Aug 8, 2020

I'd like to merge this tomorrow, it would be great to get a few more approvals if possible.

Trott pushed a commit that referenced this pull request Aug 9, 2020
@guybedford @Trott
module: use cjsCache over esm injection
0f4b4ea 
PR-URL: #34605
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Trott
Copy link
Member

Trott commented Aug 9, 2020

Landed in 0f4b4ea

@Trott Trott closed this Aug 9, 2020
MylesBorins pushed a commit that referenced this pull request Aug 17, 2020
@guybedford @MylesBorins
module: use cjsCache over esm injection
4022205 
PR-URL: #34605
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@danielleadams danielleadams mentioned this pull request Aug 20, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
@guybedford @BethGriggs
module: use cjsCache over esm injection
9a7c87d 
PR-URL: #34605
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
guybedford added a commit to guybedford/node that referenced this pull request Sep 28, 2020
@guybedford
module: use cjsCache over esm injection
39a42b3 
PR-URL: nodejs#34605
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
codebytere pushed a commit that referenced this pull request Oct 1, 2020
@guybedford @codebytere
module: use cjsCache over esm injection
3ad146d 
PR-URL: #34605
Backport-PR-URL: #35385
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@codebytere codebytere mentioned this pull request Oct 4, 2020
Merged
joyeecheung added a commit to joyeecheung/node that referenced this pull request Dec 14, 2023
@joyeecheung
module: move the CJS exports cache to internal/modules/cjs/loader
12234a0 
This puts it together with the cjsParseCache and reduces the
circular dependency on the singleton loader, which is the
only place where this cache is stored.

Drive-by: remove always-false module status check because there's
no longer a local module variable after
nodejs#34605 which is now invalid
leftover code at this point and only doesn't throw because
we happen to have a top-level variable called module.
@joyeecheung joyeecheung mentioned this pull request Dec 14, 2023
Closed
joyeecheung added a commit to joyeecheung/node that referenced this pull request Dec 14, 2023
@joyeecheung
module: move the CJS exports cache to internal/modules/cjs/loader
f2a3d8e 
This puts it together with the cjsParseCache and reduces the
circular dependency on the singleton loader, which is the
only place where this cache is stored.

Drive-by: remove always-false module status check because there's
no longer a local module variable after
nodejs#34605 which is now invalid
leftover code at this point and only doesn't throw because
we happen to have a top-level variable called module.
joyeecheung added a commit to joyeecheung/node that referenced this pull request Dec 15, 2023
@joyeecheung
module: move the CJS exports cache to internal/modules/cjs/loader
3d01dbb 
This puts it together with the cjsParseCache and reduces the
circular dependency on the singleton loader, which is the
only place where this cache is stored.

Drive-by: remove always-false module status check because there's
no longer a local module variable after
nodejs#34605 which is now invalid
leftover code at this point and only doesn't throw because
we happen to have a top-level variable called module.
jasnell pushed a commit that referenced this pull request Dec 22, 2023
@joyeecheung @jasnell
module: move the CJS exports cache to internal/modules/cjs/loader
9c5ef11 
This puts it together with the cjsParseCache and reduces the
circular dependency on the singleton loader, which is the
only place where this cache is stored.

Drive-by: remove always-false module status check because there's
no longer a local module variable after
#34605 which is now invalid
leftover code at this point and only doesn't throw because
we happen to have a top-level variable called module.

PR-URL: #51157
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024
@joyeecheung @RafaelGSS
module: move the CJS exports cache to internal/modules/cjs/loader
b621ada 
This puts it together with the cjsParseCache and reduces the
circular dependency on the singleton loader, which is the
only place where this cache is stored.

Drive-by: remove always-false module status check because there's
no longer a local module variable after
#34605 which is now invalid
leftover code at this point and only doesn't throw because
we happen to have a top-level variable called module.

PR-URL: #51157
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@joyeecheung @richardlau
module: move the CJS exports cache to internal/modules/cjs/loader
d8da197 
This puts it together with the cjsParseCache and reduces the
circular dependency on the singleton loader, which is the
only place where this cache is stored.

Drive-by: remove always-false module status check because there's
no longer a local module variable after
#34605 which is now invalid
leftover code at this point and only doesn't throw because
we happen to have a top-level variable called module.

PR-URL: #51157
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95 aduh95 aduh95 left review comments

@Trott Trott Trott approved these changes

+1 more reviewer

@bmeck bmeck bmeck approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

esm Issues and PRs related to the ECMAScript Modules implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@guybedford @nodejs-github-bot @devsnek @Trott @bmeck @aduh95 @addaleax