module: move the CJS exports cache to internal/modules/cjs/loader#51157
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot commented Dec 14, 2023
Review requested:
|
nodejs-github-bot added esm 
joyeecheungforce-pushed the cache-refactor branch from 12234a0 to f2a3d8eCompare
github-actionsbot removed the request-ci nodejs-github-bot commented Dec 14, 2023
ljharb left a comment
ljharb left a comment There was a problem hiding this comment.
other than the linter warnings, this LGTM
GeoffreyBooth left a comment
GeoffreyBooth left a comment There was a problem hiding this comment.
Lately we’ve been moving things that are shared between loaders into https://github.com/nodejs/node/tree/main/lib/internal/modules, like you can see package_json_reader.js there. That’s generally been the organization that we’ve been trying to move toward, so that we can leave the CJS loader mostly untouched and also removable once the ESM loader can replace it. The direction we’re trying to move is to reduce (eventually eliminate) dependencies of the ESM loader on the CJS loader; things that both loaders need such as the package.json reader and so on would live outside of either loader’s folder.
I think you can just move both cjsParseCache and cjsExportsCache when that happens. For now it's just clearer for them to live together. Also I don't think you need to move the code from the cjs loader to anywhere for that purpose. You can just move the shared esm bits (just the package import/export stuff) needed by the CJS loader to a helper, and let the ESM loader reference the CJS loader but not the other way around. The only real dependency the CJS loader has on ESM is for the dynamic import callback, but notice that we are going to redesign the experimental API anyway once V8 fixes the host defined options management because the current API is flawed and leaks, so the callback should be specified differently eventually. That's likely to be a native level callback configured at bootstrap time instead of something you have to reference from the JS side of the CJS loader, so at that point the JS part of the CJS loader no longer needs to reference ESM anyway. Another option is to just port the package import/export stuff to C++ and the CJS loader only needs to reference some C++ binding which is the best clarity and performance-wise IMO because that's mostly computation. So there are many ways to do it and I don't see the "moving shared JS bits to another JS file in |
github-actionsbot removed the request-ci nodejs-github-bot commented 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.
joyeecheungforce-pushed the cache-refactor branch from 97b58d8 to 3d01dbbComparegithub-actionsbot removed the request-ci 
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]>
jasnell commented Dec 22, 2023
Landed in 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]>
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]>
6 participants
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.