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.

Sign up for GitHub

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

Clarify behavior of require('http') and require('node:http') with existing require.cache entries #52992

Closed
wants to merge 1 commit into from
Closed

Clarify behavior of require('http') and require('node:http') with existing require.cache entries #52992

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions doc/api/modules.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,8 +405,13 @@ always return the built in HTTP module, even if there is `require.cache` entry
by that name.

Some core modules are always preferentially loaded if their identifier is
passed to `require()`. For instance, `require('http')` will always
return the built-in HTTP module, even if there is a file by that name. The list
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 specified without the `node:` prefix are served from the
cache if an entry for the module exists. Because cache entries can be manually
altered by modifying the `require.cache `object, this allows the module's
implementation to be replaced. To ensure `require() ` returns the original
implementation, use the `node:` prefix. The list
Comment on lines +408 to +414
Copy link
Contributor

@aduh95 aduh95 May 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be more direct, at the risk of being vague: mutation of the module cache is a thing in CJS, but that shouldn't matter for most users – and for those for which it does matter, it seems fair to assume they won't need the extra details.

Suggested change
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 specified without the `node:` prefix are served from the
cache if an entry for the module exists. Because cache entries can be manually
altered by modifying the `require.cache `object, this allows the module's
implementation to be replaced. To ensure `require() ` returns the original
implementation, use the `node:` prefix. The list
passed to `require()`. For instance, assuming no mutation of the module cache, `require('http')`
will return the built-in HTTP module, even if there is a file by that name. The list

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a newcomer to this, I find the wording "assuming no mutation of the module cache" somewhat confusing.
My goal is to clearly understand the specification, and this phrase doesn't help clarify the situation for me.
I believe my initial wording is preferable because it explicitly states the technical condition under which the behavior differs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is it confusing, and how can we clear up that confusion?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the wording "assuming no mutation of the module cache" somewhat confusing because it implies that mutations exist, which begs the question: "under what circumstances?" My wording describes the specification as it is. As for how to move forward, feel free to decide how to write this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's fine, the rest of the documentation should contain the answer to that question. Maybe it's clearer if we say require.cache instead of "the module cache"

Suggested change
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 specified without the `node:` prefix are served from the
cache if an entry for the module exists. Because cache entries can be manually
altered by modifying the `require.cache `object, this allows the module's
implementation to be replaced. To ensure `require() ` returns the original
implementation, use the `node:` prefix. The list
passed to `require()`. For instance, assuming `require.cache` is not modified, `require('http')`
will return the built-in HTTP module, even if there is a file by that name. The list

If you look at the section just above this one, it already deals with how caching works in CJS

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it should be past tense, 'assuming require.cache has not been modified'

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, I initially raised this question in the IRC, and the reply I got indicated that there is an impression that core modules are not served from the cache at all, which suggests that this is not so clear. Just my 2c.

of core modules that can be loaded without using the `node:` prefix is exposed
as [`module.builtinModules`][].

Expand Down
Loading