module: support custom paths to require.resolve() by cjihrig · Pull Request #16397 · nodejs/node · GitHub

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 single if statement, so the impact on code that doesn't use this feature should be extremely minimal.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

module

What about just having prepend and append contain the paths themselves?

How about having just append, with append on true, prepend on false, discard override defaults on undefined?

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']
}

(heh, just saw that @mscdex suggested the same thing above :-) ...)

I think that is the suggestion that @mscdex made. I plan to update to that.

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

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.

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 });

Updating this PR now...

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
#

The require() machinery checks the node_modules directory in the parent, which is how it finds bar.js.

ah, ok - got it, thanks!

What about just having prepend and append contain the paths themselves?

SGTM. I really like that idea.

Should this be something like #### require.resolve(name[, options]), with name type and meaning description? It seems to be missing in the previous variant.

+ 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.

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.

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.

A nit: String -> string

I'm not sure it matters to compare strictly with a boolean like this like we do for other special values? /cc @bmeurer

This will generate the same code as if (!paths.includes(path)), since TurboFan knows that Array#includes returns a boolean.

Ah ok, then yes, let's switch these to the if (!paths.includes(path)) style here and elsewhere.

@mscdex done.

Generally LGTM so long as others are happy with it. I didn't see anything that stood out as problematic.

CI: https://ci.nodejs.org/job/node-test-pull-request/10930/

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 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 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.

nodejs-github-bot added the module Issues and PRs related to the module subsystem. label Oct 23, 2017

gireeshpunathil reviewed Oct 23, 2017

View reviewed changes

vsemozhetbyt reviewed Oct 23, 2017

View reviewed changes

zkochan reviewed Oct 23, 2017

View reviewed changes

cjihrig force-pushed the 5963 branch from d92c538 to 5fc20c1 Compare October 23, 2017 21:22

vsemozhetbyt reviewed Oct 23, 2017

View reviewed changes

mscdex reviewed Oct 23, 2017

View reviewed changes

cjihrig force-pushed the 5963 branch 2 times, most recently from 84e4c7b to cb0fe32 Compare October 24, 2017 14:26

jasnell approved these changes Oct 24, 2017

View reviewed changes

gireeshpunathil approved these changes Oct 25, 2017

View reviewed changes

cjihrig added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 25, 2017

cjihrig force-pushed the 5963 branch from cb0fe32 to 9c6f6b0 Compare October 25, 2017 13:16

cjihrig merged commit 9c6f6b0 into nodejs:master Oct 25, 2017

cjihrig deleted the 5963 branch October 25, 2017 13:17

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

cjihrig mentioned this pull request Oct 30, 2017

string_decoder: make write after end to reset #16594

Closed

4 tasks

gibfahn mentioned this pull request Oct 31, 2017

v8.9.0 proposal #16630

Merged

tamlyn mentioned this pull request Nov 17, 2017

Request: Allow specifying lookup paths for require.resolve #5963

Closed

cjihrig mentioned this pull request Dec 19, 2017

module: replace default paths in require.resolve() #17113

Merged

3 tasks

gibfahn added dont-land-on-v6.x and removed dont-land-on-v8.x labels Jan 15, 2018

shinnn mentioned this pull request Jan 18, 2018

require.resolve.paths(${built-in module id}) returns null #18230

Closed

ExE-Boss mentioned this pull request Feb 19, 2020

esm: implement import.meta.resolve and make nodejs: scheme public #31032

Closed

4 tasks

module: support custom paths to require.resolve() by cjihrig · Pull Request #16397 · nodejs/node · GitHub

Uh oh!

Uh oh!

module: support custom paths to require.resolve() - #16397

module: support custom paths to require.resolve()#16397cjihrig merged 1 commit intonodejs:masternodejs/node:masterfrom cjihrig:5963cjihrig/node:5963Copy head branch name to clipboard

Conversation

cjihrig commented Oct 23, 2017

Checklist

Affected core subsystem(s)

Uh oh!

mscdex commented Oct 23, 2017

Uh oh!

gireeshpunathil Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Oct 23, 2017

Uh oh!

vsemozhetbyt Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Oct 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Oct 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Oct 24, 2017

Uh oh!

geek commented Oct 24, 2017

Uh oh!

gibfahn commented Jan 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

module: support custom paths to require.resolve()#16397
cjihrig merged 1 commit into
nodejs:masterfrom
cjihrig:5963

gireeshpunathil Oct 23, 2017 •
edited
Loading

vsemozhetbyt Oct 23, 2017 •
edited
Loading

vsemozhetbyt Oct 23, 2017 •
edited
Loading