-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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 loading implementation does not match API docs #3873
Comments
The documentation was correct at the beginning see this commit but changed at some point. |
@targos good find. The older documentation describes this case as an 'optimization', although that's a questionable label given that it materially changes the API's contract. |
We'll probably need to update the docs. Whatever changed, we probably can't change back. |
Let's just add back the original:
|
PR: #3920 |
The module loading system will not append node_modules to a path already ending in node_modules. This used to be documented, but it was lost. Fixes: #3873 PR-URL: #3920 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
The module loading system will not append node_modules to a path already ending in node_modules. This used to be documented, but it was lost. Fixes: #3873 PR-URL: #3920 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
The module loading system will not append node_modules to a path already ending in node_modules. This used to be documented, but it was lost. Fixes: #3873 PR-URL: #3920 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
The module loading system will not append node_modules to a path already ending in node_modules. This used to be documented, but it was lost. Fixes: #3873 PR-URL: #3920 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
The module loading system will not append node_modules to a path already ending in node_modules. This used to be documented, but it was lost. Fixes: #3873 PR-URL: #3920 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
The API Documentation for modules states:
In the given example, a call to
require('bar.js')
inside a file at'/home/ry/projects/foo.js'
would search the following locations:The documented lookup procedure makes no qualifications about the names of parent directories, so the example's
ry
andprojects
directory names are presumably arbitrary. But if we changery
to'node_modules'
then the lookup is materially different:Note that the location
'/home/node_modules/node_modules/bar.js'
is not considered.The API documentation seems to diverge slightly from the implementation here. Perhaps one should be amended to match the other?
Example
This is just for the curious; the point of this issue is that the module loading documentation and implementation diverge in this case.
This came up in a discussion about an undesirable consequence of
npm3
's flattening behaviour, namely that it makes it easy to ship buggy code that forgets to declare some dependencies in itspackage.json
, but still works most of the time because one or more other dependencies require the missing dependency so it is installed at the top level anyway (due to flatnpm install
).For example, suppose a module at
/home/foo
declares a dependency onfs-extra
. Later on, it starts usingrimraf
as well but forgets to add it as a dependency. However it just works (usually) becausefs-extra
depends onrimraf
and sonpm3
(usually) installs it at'/home/foo/node_modules/rimraf'
. Note the usually because the behaviour is also a function of other factors.A suggestion to catch this bug early was to put indirect dependencies one level deeper. So the example would look like:
According to the modules API documentation, the expected behaviour is that
fs-extra
can successfullyrequire('rimraf')
, but iffoo
tries torequire('rimraf')
it fails.The actual behaviour is that
require('rimraf')
fails both forfs-extra
and forfoo
.The text was updated successfully, but these errors were encountered: