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

cjihrig · 2017-10-23T02:03:42Z

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

mscdex · 2017-10-23T04:15:22Z

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

gireeshpunathil · 2017-10-23T06:32:37Z

doc/api/modules.md

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


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

gireeshpunathil · 2017-10-23T06:35:38Z

test/fixtures/require-resolve.js

+  // Resolution from nested index directory also checks node_modules.
+  assert.strictEqual(
+    require.resolve('bar', { paths: [nestedIndex] }),
+    path.join(nodeModules, 'bar.js')


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!

cjihrig · 2017-10-23T12:22:53Z

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

SGTM. I really like that idea.

vsemozhetbyt · 2017-10-23T16:57:40Z

doc/api/modules.md

@@ -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])


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.

jasnell · 2017-10-23T17:20:52Z

big +1 to this idea, left a suggestion on the impl tho.

zkochan · 2017-10-23T18:08:10Z

lib/module.js

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


Seems like this should not be executed if options.prepend or options.append is true

cjihrig · 2017-10-23T21:26:46Z

Updated with the discussed implementation of having separate set and get functionality. Also added duplicate path removal logic.

vsemozhetbyt · 2017-10-23T21:35:15Z

doc/api/modules.md

+added: REPLACEME
+-->
+
+* `request` {String} The module path whose lookup paths are being retrieved.


A nit: String -> string

mscdex · 2017-10-23T22:16:19Z

lib/module.js

+      const path = options.paths[i];
+      const lookupPaths = Module._resolveLookupPaths(path, parent, true);
+
+      if (paths.includes(path) === false)


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.

jasnell

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

cjihrig · 2017-10-24T14:55:45Z

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

geek · 2017-10-24T15:01:37Z

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>

gibfahn · 2018-01-15T22:34:05Z

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cjihrig commented Oct 23, 2017

mscdex commented Oct 23, 2017

gireeshpunathil Oct 23, 2017 •

edited

Loading

jasnell Oct 23, 2017

jasnell Oct 23, 2017

cjihrig Oct 23, 2017

zkochan Oct 23, 2017

cjihrig Oct 23, 2017

jasnell Oct 23, 2017

cjihrig Oct 23, 2017

gireeshpunathil Oct 23, 2017

cjihrig Oct 23, 2017

gireeshpunathil Oct 23, 2017

cjihrig commented Oct 23, 2017

vsemozhetbyt Oct 23, 2017 •

edited

Loading

vsemozhetbyt Oct 23, 2017 •

edited

Loading

cjihrig Oct 23, 2017

jasnell commented Oct 23, 2017

zkochan Oct 23, 2017

cjihrig commented Oct 23, 2017

vsemozhetbyt Oct 23, 2017

mscdex Oct 23, 2017

bmeurer Oct 24, 2017

mscdex Oct 24, 2017

cjihrig Oct 24, 2017

jasnell left a comment

cjihrig commented Oct 24, 2017

geek commented Oct 24, 2017

gibfahn commented Jan 15, 2018

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

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

Conversation

cjihrig commented Oct 23, 2017

Checklist

Affected core subsystem(s)

mscdex commented Oct 23, 2017

gireeshpunathil Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjihrig commented Oct 23, 2017

vsemozhetbyt Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

vsemozhetbyt Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Oct 23, 2017

Choose a reason for hiding this comment

cjihrig commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

cjihrig commented Oct 24, 2017

geek commented Oct 24, 2017

gibfahn commented Jan 15, 2018

gireeshpunathil Oct 23, 2017 •

edited

Loading

vsemozhetbyt Oct 23, 2017 •

edited

Loading

vsemozhetbyt Oct 23, 2017 •

edited

Loading