module: move the CJS exports cache to internal/modules/cjs/loader #51157
Conversation
|
Review requested:
|
nodejs-github-bot
added
esm
joyeecheung
force-pushed
the
cache-refactor
branch
from
12234a0 to
f2a3d8e
Compare
github-actions
bot
removed
the
request-ci
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-actions
bot
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 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
force-pushed
the
cache-refactor
branch
from
97b58d8 to
3d01dbb
Compare
github-actions
bot
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]>
|
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]>
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.