-
Notifications
You must be signed in to change notification settings - Fork 28.2k
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
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
Clarification Needed: require('http') and require('node:http') with existing require.cache entries #52970
Comments
@nodejs/loaders |
If you populate |
@aduh95 why is that allowed for preferentially loaded modules ? |
I don’t understand the question, what is allowed? |
As your example demonstrates, manually modifying the cache entry for preferentially loaded core modules bypasses the intended preferential loading characteristics. This seems counterproductive to the purpose of preferential loading. |
IIUC if you have a dependency named |
I don’t know the reason, and it doesn’t really matter, that behavior cannot be changed without breaking the ecosystem. If you don’t like that behavior, you can add the |
IIUC, the goal of preferential loading is to 'protect' core modules from being overwritten. Allowing manual manipulation of their cache entries seems to defeat this purpose. |
Is manually manipulating cache entries for core modules a common scenario? |
Like I said, the “goal” does not really matter, it is the way it is, and changing it is simply not worth it at this point. As someone who wasn’t involved at all in the development of CJS, I can’t tell you what was the rationale behind this design choice – or maybe it was an oversight, again, I have no idea – but as a maintainer I can tell you that whatever it is, it won’t change the fact that we want maximum stability on that area of the code base, and any PR that would try to amend the behavior would likely get rejected. |
Then, at the very least, I suggest amending the documentation to clarify the behavior, with something like: "Some core modules are always preferentially loaded if their identifier is passed to require(). For instance, require('http') will return the built-in HTTP module, even if there is a file by that name. However, note that core modules imported without the node: prefix are served from the cache if an entry for the module exists. Since cache entries of core modules can be manually manipulated, this allows replacing the implementation for the dependent module. If you want to ensure obtaining the original implementation, use the node: prefix." |
I’m not sure “the original implementation” is the correct phrasing, as it’s still possible to overwrite built-in modules as well (e.g. you could have |
read: "If you want to ensure obtaining the original implementation" Alternatively: "If you want to ensure I'm fine with either. What do you prefer?
Sure. I can give it a try :) |
However, since core modules are also "imported" I think "obtaining" is better. |
If you use |
Aren't core modules considered CommonJS modules? Isn't the CommonJS cache still accessible from an ES module? |
No they considered… built-in modules. You can certainly access the require cache from ESM, but that has no effect on the matter. |
IMO this should be converted to a discussion, am I okay to do that? |
I don't understand. Are you saying that the require cache can't be manipulated in an ES module in the same way shown in the code you posted? |
@RedYetiDev I have no objection. |
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
Affected URL(s)
https://nodejs.org/docs/latest-v20.x/api/modules.html#core-modules
Description of the problem
According to the documentation:
And:
Given this, if require('http') always returns the built-in HTTP module, under what circumstances would a require.cache entry by that name exist, making it necessary to use require('node:http')?
The text was updated successfully, but these errors were encountered: