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

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

Closed
nzakas opened this issue Mar 30, 2016 · 29 comments
Closed

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

nzakas opened this issue Mar 30, 2016 · 29 comments
Labels
feature request Issues that request new features to be added to Node.js. module Issues and PRs related to the module subsystem.

Comments

@nzakas
Copy link

nzakas commented Mar 30, 2016

tl;dr I'd like to propose an additional API be exposed to Node.js JavaScript that allows developers to perform require.resolve(), but with an custom list of lookup paths.

Background

In ESLint, we allow people to extend their configuration files with Node.js packages, so you can do something like this:

extends: airbnb

And then ESLint requires eslint-config-airbnb. That worked fine until people started creating shareable configs, published on npm, that wanted to inherit from other shareable configs (also published on npm). In that case, they'd want the require lookup to happen from their config file and not from the ESLint file doing the require.

Our Solution

What we ended up doing to get this to work: https://github.com/eslint/eslint/blob/master/lib/util/module-resolver.js

In short:

  1. Create the normal array of lookup paths by combining Module.globalPaths and module.paths
  2. Prepend another path to the front of that array
  3. Use Module._findPath() to do the lookup, passing the lookup paths array

The obvious ugliness here is that we're not just relying on Module, but we're relying on Module._findPath(), which seems to indicate that this method should not be used (assuming the underscore is intended to mean "private".

What I'd Like

Some way to get the functionality of Module._findPath() that is an official Node.js API that we can rely on without fear of it changing or being removed. Some possible options (definitely not exhaustive):

  1. Allow a second argument to require.resolve() that allows you to pass an array of lookup paths.
  2. Bless Module._findPath() by creating something like Module.resolveFromPaths() that calls Module._findPath() under the covers

Of course, these are just a few ideas I had in my head. Anything that accomplishes the same functionality would be awesome.

Prior Art

It seems like there's a larger need for this capability based on modules available on npm:

  1. resolve - effectively does the same thing by trying to recreate the Node.js lookup process. We tried using this, but there are enough differences with the real Node.js implementation that we abandoned it. It also hardcodes some information that should be dynamic (like core module names). 7 million downloads in the past month.
  2. resolve-module - a simpler and less popular version of the same thing.
  3. custom-resolve - a customized version of resolve.
  4. resolve-cwd - require.resolve using CWD as the root. 50,000 downloads in the past month.
  5. resolve-from - require.resolve from any arbitrary path. 1.5 million downloads in the past month.
@vkurchatkin vkurchatkin added module Issues and PRs related to the module subsystem. feature request Issues that request new features to be added to Node.js. labels Mar 30, 2016
@benjamingr
Copy link
Member

Sorry, Module is locked. I don't think this can happen. I think it's worth discussing regardless.


I'm not sure I understand your particular problem, is this not something module.require (and not require) would solve for you?

If you don't want to require the actual file - is this something a module.require.resolve would fix for you?

@nzakas
Copy link
Author

nzakas commented Mar 31, 2016

require.resolve could handle it if it could be given different lookup paths. We use the full path to various references that get passed around multiple files, so where we resolve the module path isn't the same place that we require it.

If Module is locked, does that mean it's safe to rely on Module._findPaths? If so, then maybe no change is needed.

@benjamingr
Copy link
Member

_findPaths is an internal method, there is no gurantee regarding it as far as I know. In practice I doubt it'll break but that's not a position I'd be comfortable to have you in (relying on it).

Would you mind elaborating a little on why you need to pass the paths inside rather than using module.require ?

@nzakas
Copy link
Author

nzakas commented Mar 31, 2016

Sure, basically, we have a configuration structure that we build up from multiple sources. At different points during that process, we convert Node.js module names into their paths, and we require those paths at different points in time. For instance, with extends, we do end up requiring fairly soon after resolving, but for parser we do not. The value of parser is passed along in the configuration (which contains only json-compatible data) until the very last moment before the JS code is parsed. At that point, it is both required to start parsing and the path is also passed along to rules in case they want to require it.

So, changing just require helps with one case but not the other.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 31, 2016

FWIW, I'd also like to bless Module._findPath(), and will gladly submit the PR to make it happen.

@Trott
Copy link
Member

Trott commented Mar 31, 2016

Putting aside the Locked status issue for now, I'd also be for it (what @cjihrig said) if we can rename it to something without a _ and keep _findPath() as an undocumented alias for backwards compatibility.

Returning to the Locked status, if we have to vote to move it to Stable in the CTC or something, that's a high bar but not an entirely insurmountable obstacle. I certainly appreciate the effort here to document the use case because that should certainly be a pre-requisite for unlocking.

@benjamingr
Copy link
Member

I think we can expose this functionality too (sans the _) as it directly relates to the internal module system and it's useful and there is no harm generally in doing it.

That said, we should also investigate what exposing it publicly would mean. What happens if someone overrides it with their own hooks? Should that change the internal reference or only further direct calls to it from outside of core?

Unlocking a module should require a very high bar.

Also - do we have any info on who else is using this from the outside?

@vkurchatkin
Copy link
Contributor

It's not clear, why is this a problem at all. Algorithm is fixed, so implementing it in userland should be possible

@cjihrig
Copy link
Contributor

cjihrig commented Mar 31, 2016

It's fixed in theory. Things do actually change in it from time to time. I think @nzakas has made a pretty compelling argument that people want this behavior. It's already exposed, so I don't think committing to it is a bad idea.

@Trott
Copy link
Member

Trott commented Mar 31, 2016

It's already exposed, so I don't think committing to it is a bad idea.

That's the crux for me. It's already exposed, people are already using it. Exposing "private" methods and expecting people not to use them is something that we should try to correct where we can, and it seems that we can here.

@vkurchatkin
Copy link
Contributor

@Trott the whole module is "private"

is something that we should try to correct where we can, and it seems that we can here.

We can't just document everything that's exposed.

@Trott
Copy link
Member

Trott commented Mar 31, 2016

@Trott the whole module is "private"

True, it's "private". It's also:

  • exposed
  • being used by people
  • something the project is saying will not change
  • the subject of a valid use case (at least for _findPath())

Those things taken as a whole seem to suggest that documenting _findPath() is both a low-risk and user-friendly approach.

There are other considerations, of course, and this may not be the right path after all. I don't see that as a foregone conclusion.

@vkurchatkin
Copy link
Contributor

exposed

A lot of things are exposed.

being used by people

People use everything they can find

something the project is saying will not change

Public APIs are not going to change, and behaviour is not going to change. Stuff like _findPath doesn't even have well defined semantics. If we just take a random internal function, make it public and commit to never change it, we might found ourselves in trouble later.

@vkurchatkin
Copy link
Contributor

I propose adding a second argument to resolve, that is used as base path, if present. @nzakas would that work for you?

@nzakas
Copy link
Author

nzakas commented Apr 1, 2016

@vkurchatkin I'm not familiar with the term "base path" in this context. What I had in mind was to allow an array of lookup paths to be passed, just like Module._findPaths. Alternately, passing an array of paths to be searched before the internal path list could work for my case, but I'm not sure if there's another case where people might want to avoid the regular lookup paths altogether.

@vkurchatkin
Copy link
Contributor

@nzakas I mean the following: when you call require.resolve('foo') it returns the path that would be used if require('foo') was used in the current file; if you call require.resolve('foo', '/some/file.js') it returns the path that would be used if require('foo') was used in /some/file.js.

@nzakas
Copy link
Author

nzakas commented Apr 1, 2016

@vkurchatkin ah! So that would then calculate what would effectively be module.paths for that base path?

@vkurchatkin
Copy link
Contributor

@nzakas yes, it will resolve paths under the hood

@nzakas
Copy link
Author

nzakas commented Apr 1, 2016

Just so I'm understanding correctly, would this add those paths to what would already be searched? Or would they replace some of the paths that would have been searched?

What I mean is, if the normal lookup is:

module.paths.concat(Module.globalPaths)

Would this change mean A or B below:

// calculatedPaths is generated from the second argument to require.resolve

// A
calculatedPaths.concat(module.paths.concat(Module.globalPaths))

// B
calculatedPaths.concat(Module.globalPaths)

@nzakas
Copy link
Author

nzakas commented Apr 13, 2016

@vkurchatkin just checking in on this.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 25, 2016

I put together cjihrig@6e72963 as a proof of concept. It allows you pass an array of paths to require.resolve(). This array can be prepended to, appended to, or replace the results of Module._resolveLookupPaths() in Module._resolveFilename(). Because this is a hot code path, I tried to reduce the impact to a single if statement when this new feature is not used.

If there is interest in this, I can add docs and tests, and create a proper PR.

@nzakas
Copy link
Author

nzakas commented Apr 26, 2016

I'm definitely still interesting in this. :)

@jasnell
Copy link
Member

jasnell commented May 17, 2016

@cjihrig ... I definitely think this is interesting but I'm also definitely sure that there will be resistance extending or changing the existing API. The good news is that everyone is very reluctant to change any part of Module so I doubt _findPaths would be going any where any time soon and if it did, there'd have to be a deprecation cycle on it.

@Trott
Copy link
Member

Trott commented Jul 7, 2017

Not sure if it makes a difference, but we got rid of the Locked status entirely. The module API is now Stable.

@cjihrig
Copy link
Contributor

cjihrig commented Jul 12, 2017

I rebased my branch, https://github.com/cjihrig/node-1/tree/5963, in case anyone wants to discuss this now.

addaleax pushed a commit to ayojs/ayo that referenced this issue 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 issue 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 issue 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 issue 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>
@tamlyn
Copy link

tamlyn commented Nov 17, 2017

I don't understand how #16397 resolves (no pun intended) this issue. I've tried doing require.resolve.paths(process.cwd()) but it doesn't return what I expected. I expected it to return more or less what Module._nodeModulePaths() does. I've made a small repo to better illustrate my confusion.

@cjihrig
Copy link
Contributor

cjihrig commented Nov 18, 2017

I took a look at your repo. Everything seems to be working as expected with one exception. Opened #17113 to address it.

@tamlyn
Copy link

tamlyn commented Nov 20, 2017

Thanks! Just built your PR and the paths option to require.resolve now works as I expected 🎉

I still don't understand the use case for require.resolve.paths as implemented but that's fine since I don't actually need it.

@cjihrig
Copy link
Contributor

cjihrig commented Nov 20, 2017

I still don't understand the use case for require.resolve.paths as implemented but that's fine since I don't actually need it.

Assume you called require.resolve(). require.resolve.paths provides a list of all of the directories that would have been searched by that resolve() call.

cjihrig added a commit to cjihrig/node that referenced this issue Nov 21, 2017
Prior to this commit, the default search paths would be included
in the require.resolve() process, even if user specified paths
were provided. This commit causes the default paths to be
omitted by using a fake parent module.

Refs: nodejs#5963
PR-URL: nodejs#17113
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this issue 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>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
Prior to this commit, the default search paths would be included
in the require.resolve() process, even if user specified paths
were provided. This commit causes the default paths to be
omitted by using a fake parent module.

Refs: #5963
PR-URL: #17113
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this issue Jan 27, 2018
Prior to this commit, the default search paths would be included
in the require.resolve() process, even if user specified paths
were provided. This commit causes the default paths to be
omitted by using a fake parent module.

Refs: #5963
PR-URL: #17113
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. module Issues and PRs related to the module subsystem.
Projects
None yet
Development

No branches or pull requests

7 participants