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.

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

Merged
merged 1 commit into from
Oct 25, 2017
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Oct 23, 2017

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

@nodejs-github-bot nodejs-github-bot added the module Issues and PRs related to the module subsystem. label Oct 23, 2017
@mscdex
Copy link
Contributor

mscdex commented Oct 23, 2017

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

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

Copy link
Member

@gireeshpunathil gireeshpunathil Oct 23, 2017

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?

Copy link
Member

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

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link

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

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

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')
Copy link
Member

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
#

Copy link
Contributor Author

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.

Copy link
Member

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!

@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 23, 2017

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

SGTM. I really like that idea.

@@ -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])
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Oct 23, 2017

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.

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Oct 23, 2017

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.

Copy link
Contributor Author

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.

@jasnell
Copy link
Member

jasnell commented Oct 23, 2017

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);
Copy link

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

@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 23, 2017

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

added: REPLACEME
-->

* `request` {String} The module path whose lookup paths are being retrieved.
Copy link
Contributor

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)
Copy link
Contributor

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

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mscdex done.

Copy link
Member

@jasnell jasnell left a 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.

@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 24, 2017

@geek
Copy link
Member

geek commented Oct 24, 2017

LGTM

@cjihrig cjihrig added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 25, 2017
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>
@cjihrig cjihrig merged commit 9c6f6b0 into nodejs:master Oct 25, 2017
@cjihrig cjihrig deleted the 5963 branch October 25, 2017 13:17
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
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>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
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>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
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>
gibfahn pushed a commit that referenced this pull request Oct 31, 2017
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>
@gibfahn gibfahn mentioned this pull request Oct 31, 2017
MylesBorins pushed a commit that referenced this pull request Oct 31, 2017
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
gibfahn added a commit that referenced this pull request Oct 31, 2017
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
gibfahn added a commit that referenced this pull request Oct 31, 2017
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
jasnell pushed a commit that referenced this pull request Oct 31, 2017
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
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
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
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
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
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
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>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
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
@gibfahn
Copy link
Member

gibfahn commented Jan 15, 2018

Release team were -1 on landing on v6.x, if you disagree let us know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants