Skip to content
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

module: use cjsCache over esm injection #34605

Closed
wants to merge 3 commits into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Aug 3, 2020

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 nodejs-github-bot added the esm Issues and PRs related to the ECMAScript Modules implementation. label Aug 3, 2020
@guybedford
Copy link
Contributor Author

//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

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.

@nodejs-github-bot
Copy link
Collaborator

@guybedford
Copy link
Contributor Author

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
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
PR-URL: #34605
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@danielleadams danielleadams mentioned this pull request Aug 20, 2020
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
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
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
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
joyeecheung added a commit to joyeecheung/node that referenced this pull request Dec 14, 2023
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 14, 2023
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
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
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
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
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
Labels
esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants