-
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: support custom paths to require.resolve() #16397
Conversation
What about just having |
doc/api/modules.md
Outdated
* `append` {boolean} If `true`, `paths` is appended to the default resolution | ||
paths instead of overwriting them. If `append` and `prepend` are both `true` | ||
then `prepend` behavior is used. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about having just append
, with append on true
, prepend on false
, discard override defaults on undefined
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we could go with an approach like
{
paths: [ 'these', 'replace', 'the', 'existing', 'path'],
prepend: [ 'these', 'prepend', 'to', 'the', 'path'],
append: [ 'these', 'append', 'to', 'the', 'path']
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(heh, just saw that @mscdex suggested the same thing above :-) ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is the suggestion that @mscdex made. I plan to update to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to get the default resolution paths? If yes, require.resolve
could just accept options.paths
. Appending/prepending/replacing can be done outside of require.resolve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Module._resolveLookupPaths()
gives the paths, but it is not public API. It does seem simplest and most flexible to provide a way to get and set the paths. I could update resolve()
in this PR to only overwrite the paths with what is passed in, dropping the append
and prepend
functionality. Then, add require.resolve.paths(request)
which returns the default paths for that request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it further, that's likely the easiest approach... specifically, have a public API for getting the current resolve path, and an option only for overriding it... e.g.
const paths = resolve.getLookupPaths(); // returns a mutable *copy*
paths.unshift('/some/other/path'); // prepend
paths.push('/another/path'); // append
resolve(name, { paths });
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating this PR now...
// Resolution from nested index directory also checks node_modules. | ||
assert.strictEqual( | ||
require.resolve('bar', { paths: [nestedIndex] }), | ||
path.join(nodeModules, 'bar.js') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something wrong here? bar.js is not present under nestedIndex
so can you double-check this check?
This is what I get for bar.js and three.js:
#find . -name bar.js
./test/fixtures/node_modules/bar.js
./test/fixtures/node_modules/node_modules/bar.js
#find . -name three.js
./test/fixtures/json-with-directory-name-module/module-stub/one/two/three.js
./test/fixtures/json-with-directory-name-module/module-stub/one-trailing-slash/two/three.js
./test/fixtures/nested-index/three.js
#
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The require()
machinery checks the node_modules
directory in the parent, which is how it finds bar.js
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, ok - got it, thanks!
SGTM. I really like that idea. |
doc/api/modules.md
Outdated
@@ -598,11 +598,26 @@ filename scales linearly with the number of registered extensions. | |||
In other words, adding extensions slows down the module loader and | |||
should be discouraged. | |||
|
|||
#### require.resolve() | |||
#### require.resolve([options]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be something like #### require.resolve(name[, options])
, with name
type and meaning description? It seems to be missing in the previous variant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+ It seems, till we automate this, we should check possible links in all docs if we change headers. Currently, we have one for this header, so it also needs updating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out. I've updated the documentation and searched for relevant links.
big +1 to this idea, left a suggestion on the impl tho. |
lib/module.js
Outdated
@@ -484,13 +484,30 @@ function tryModuleLoad(module, filename) { | |||
} | |||
} | |||
|
|||
Module._resolveFilename = function(request, parent, isMain) { | |||
Module._resolveFilename = function(request, parent, isMain, options) { | |||
if (NativeModule.nonInternalExists(request)) { | |||
return request; | |||
} | |||
|
|||
var paths = Module._resolveLookupPaths(request, parent, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this should not be executed if options.prepend
or options.append
is true
Updated with the discussed implementation of having separate set and get functionality. Also added duplicate path removal logic. |
doc/api/modules.md
Outdated
added: REPLACEME | ||
--> | ||
|
||
* `request` {String} The module path whose lookup paths are being retrieved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A nit: String -> string
lib/module.js
Outdated
const path = options.paths[i]; | ||
const lookupPaths = Module._resolveLookupPaths(path, parent, true); | ||
|
||
if (paths.includes(path) === false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it matters to compare strictly with a boolean like this like we do for other special values? /cc @bmeurer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will generate the same code as if (!paths.includes(path))
, since TurboFan knows that Array#includes
returns a boolean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, then yes, let's switch these to the if (!paths.includes(path))
style here and elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mscdex done.
84e4c7b
to
cb0fe32
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM so long as others are happy with it. I didn't see anything that stood out as problematic.
LGTM |
This commit allows custom lookup paths to be passed to require.resolve(). It also adds require.resolve.paths() which retrieves the default resolution paths. Fixes: nodejs#5963 Fixes: nodejs#16389 PR-URL: nodejs#16397 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Wyatt Preul <wpreul@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
This commit allows custom lookup paths to be passed to require.resolve(). It also adds require.resolve.paths() which retrieves the default resolution paths. Fixes: nodejs/node#5963 Fixes: nodejs/node#16389 PR-URL: nodejs/node#16397 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Wyatt Preul <wpreul@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
This commit allows custom lookup paths to be passed to require.resolve(). It also adds require.resolve.paths() which retrieves the default resolution paths. Fixes: #5963 Fixes: #16389 PR-URL: #16397 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Wyatt Preul <wpreul@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
This commit allows custom lookup paths to be passed to require.resolve(). It also adds require.resolve.paths() which retrieves the default resolution paths. Fixes: #5963 Fixes: #16389 PR-URL: #16397 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Wyatt Preul <wpreul@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
This commit allows custom lookup paths to be passed to require.resolve(). It also adds require.resolve.paths() which retrieves the default resolution paths. Fixes: #5963 Fixes: #16389 PR-URL: #16397 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Wyatt Preul <wpreul@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
This LTS release comes with 87 commits. This includes 30 that are updates to lib/ or src/, 20 that are test related, 13 that are doc related, 19 which are build / tools related, and 4 commits which are updates to dependencies. Notable Changes: * doc: - add Gibson Fahnestock to Release team (Gibson Fahnestock) #16620 * deps: - update npm to 5.5.1 (Myles Borins) #16509 * http2: - The exposed http2 socket is no longer manipulatable (Anatoli Papirovski) #16330 * module: - support custom paths to require.resolve() (cjihrig) #16397 * util: - util.TextEncoder and util.TextDecoder are no longer experimental. There will no longer be a warning when they are used (James M Snell) #15743 PR-URL: #16630
This LTS release comes with 87 commits. This includes 30 that are updates to lib/ or src/, 20 that are test related, 13 that are doc related, 19 which are build / tools related, and 4 commits which are updates to dependencies. Notable Changes: * doc: - add Gibson Fahnestock to Release team (Gibson Fahnestock) #16620 * deps: - update npm to 5.5.1 (Myles Borins) #16509 * http2: - The exposed http2 socket is no longer manipulatable (Anatoli Papirovski) #16330 * module: - support custom paths to require.resolve() (cjihrig) #16397 * util: - util.TextEncoder and util.TextDecoder are no longer experimental. There will no longer be a warning when they are used (James M Snell) #15743 PR-URL: #16630
This LTS release comes with 87 commits. This includes 30 that are updates to lib/ or src/, 20 that are test related, 13 that are doc related, 19 which are build / tools related, and 4 commits which are updates to dependencies. Notable Changes: * doc: - add Gibson Fahnestock to Release team (Gibson Fahnestock) #16620 * deps: - update npm to 5.5.1 (Myles Borins) #16509 * http2: - The exposed http2 socket is no longer manipulatable (Anatoli Papirovski) #16330 * module: - support custom paths to require.resolve() (cjihrig) #16397 * util: - util.TextEncoder and util.TextDecoder are no longer experimental. There will no longer be a warning when they are used (James M Snell) #15743 PR-URL: #16630
This LTS release comes with 87 commits. This includes 30 that are updates to lib/ or src/, 20 that are test related, 13 that are doc related, 19 which are build / tools related, and 4 commits which are updates to dependencies. Notable Changes: * doc: - add Gibson Fahnestock to Release team (Gibson Fahnestock) #16620 * deps: - update npm to 5.5.1 (Myles Borins) #16509 * http2: - The exposed http2 socket is no longer manipulatable (Anatoli Papirovski) #16330 * module: - support custom paths to require.resolve() (cjihrig) #16397 * util: - util.TextEncoder and util.TextDecoder are no longer experimental. There will no longer be a warning when they are used (James M Snell) #15743 PR-URL: #16630
This LTS release comes with 87 commits. This includes 30 that are updates to lib/ or src/, 20 that are test related, 13 that are doc related, 19 which are build / tools related, and 4 commits which are updates to dependencies. Notable Changes: * doc: - add Gibson Fahnestock to Release team (Gibson Fahnestock) nodejs/node#16620 * deps: - update npm to 5.5.1 (Myles Borins) nodejs/node#16509 * http2: - The exposed http2 socket is no longer manipulatable (Anatoli Papirovski) nodejs/node#16330 * module: - support custom paths to require.resolve() (cjihrig) nodejs/node#16397 * util: - util.TextEncoder and util.TextDecoder are no longer experimental. There will no longer be a warning when they are used (James M Snell) nodejs/node#15743 PR-URL: nodejs/node#16630
This LTS release comes with 87 commits. This includes 30 that are updates to lib/ or src/, 20 that are test related, 13 that are doc related, 19 which are build / tools related, and 4 commits which are updates to dependencies. Notable Changes: * doc: - add Gibson Fahnestock to Release team (Gibson Fahnestock) nodejs/node#16620 * deps: - update npm to 5.5.1 (Myles Borins) nodejs/node#16509 * http2: - The exposed http2 socket is no longer manipulatable (Anatoli Papirovski) nodejs/node#16330 * module: - support custom paths to require.resolve() (cjihrig) nodejs/node#16397 * util: - util.TextEncoder and util.TextDecoder are no longer experimental. There will no longer be a warning when they are used (James M Snell) nodejs/node#15743 PR-URL: nodejs/node#16630
This commit allows custom lookup paths to be passed to require.resolve(). It also adds require.resolve.paths() which retrieves the default resolution paths. Fixes: nodejs/node#5963 Fixes: nodejs/node#16389 PR-URL: nodejs/node#16397 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Wyatt Preul <wpreul@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
This LTS release comes with 87 commits. This includes 30 that are updates to lib/ or src/, 20 that are test related, 13 that are doc related, 19 which are build / tools related, and 4 commits which are updates to dependencies. Notable Changes: * doc: - add Gibson Fahnestock to Release team (Gibson Fahnestock) nodejs/node#16620 * deps: - update npm to 5.5.1 (Myles Borins) nodejs/node#16509 * http2: - The exposed http2 socket is no longer manipulatable (Anatoli Papirovski) nodejs/node#16330 * module: - support custom paths to require.resolve() (cjihrig) nodejs/node#16397 * util: - util.TextEncoder and util.TextDecoder are no longer experimental. There will no longer be a warning when they are used (James M Snell) nodejs/node#15743 PR-URL: nodejs/node#16630
Release team were -1 on landing on v6.x, if you disagree let us know. |
I originally wrote this for #5963, but didn't open a PR. Today, it was requested again in #16389, so I thought I'd open a PR. I'm not sure how it will be received.
This PR supports passing custom paths to
require.resolve()
. The custom paths can replace the default paths, be prepended to the default paths, or be appended to the default paths. All of the functionality is isolated to a singleif
statement, so the impact on code that doesn't use this feature should be extremely minimal.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
module