feat(node-resolve): add new option, modulePaths

[ajhyndman]

Rollup Plugin Name: plugin-node-resolve

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

This change introduces a new option for @rollup/plugin-node-resolve: modulePaths.

Unlike moduleDirectories, modulePaths allows the user to specify an absolute path to alternative locations (in addition to the importer's local directory) that should be searched for node modules.

This can be useful (for example) to support a CI system that caches node_modules in a non-standard location.

In node, this is supported via the NODE_PATH environment variable. Conveniently, this also brings @rollup/plugin-node-resolve into alignment with Jest's configuration options. See: https://jestjs.io/docs/configuration#modulepaths-arraystring

[shellscape]

@tjenkinson please have a peek at this one

[tjenkinson]

thanks @ajhyndman!

Looks good. Got a few suggestions

[tjenkinson]

I think it's good to test this but it would be nice to also actually flag this as a proper error if the user tries to provide a path instead of name for this option.

Please could we update the code to something like:

if (moduleDirectories.some((name) => name.includes('/'))) {
  throw new Error(
    '`moduleDirectories` option must only contain directory names. If you want to load modules from a custom directory see `modulePaths`.'
  );
}

in index.js

Then this test can just check that

nodeResolve({
  moduleDirectories: ['some/path']
})

throws that error

[ajhyndman]

Yeah, that sounds awesome! Would this change be breaking though? Should it require a major version bump? I'm concerned that there may be a large subset of users (like us) relying on the existing behaviour.

[tjenkinson]

If you use a path doesn’t it just get ignored currently?

If that’s the case and it silently fails (with the warning when module not found) IMO this doesn’t need to be a breaking change as it’s highlighting an invalid config

[tjenkinson]

What’s the existing behaviour you’re relying on?

[ajhyndman]

If you pass a relative path, it appears that the same logic for folder names is applied. i.e. When searching for node modules, rollup will recursively walk up the filesystem, at each level looking for the full subpath specified.

I think we inadvertently assumed that moduleDirectories would allow us to specify a NODE_PATH, and for a little while we were accidentally getting away with it.

[tjenkinson]

Interesting. This readme says it's meant to just contain directory names, and the doc for the resolve package we use says

opts.moduleDirectory - directory (or directories) in which to recursively look for modules. default: "node_modules"

and the tests for it are only for names.

Given that, I think the fact that it did kind of work with a path was unintentional/undocumented, and therefore IMO erroring would be better and this would be fine as a patch. Especially given if people were misusing moduleDirectories when they see the error there will be the correct option available for them to to move too.

@shellscape might have a different opinion though :)

[shellscape]

sounds reasonable to me

[ajhyndman]

I have rebased and pushed a change addressing this feedback. Let me know what you think!

[tjenkinson]

Sorry missed your reply

[shellscape]

@tjenkinson PR is awaiting your review of most recent changes in response to your comments. please take a look.

[tjenkinson]

Lgtm!

[shellscape]