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

fix(node-resolve): modulePaths default is not set #1534

Merged
merged 3 commits into from
Oct 8, 2023

Conversation

bytestream
Copy link
Contributor

Rollup Plugin Name: 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

https://github.com/rollup/plugins/tree/master/packages/node-resolve#modulepaths states that the default modulePaths value is []. This is not true, the default is null. This results in the resolve package scanning additional directories such as /$HOME/.node_modules - browserify/resolve#273

Setting nodeResolve({modulePaths: []}) prevents /$HOME/.node_modules from being scanned.

I suggest either the default in the README is changed, or modulePaths is correctly set to match the README.

@tada5hi
Copy link
Member

tada5hi commented Jul 8, 2023

you got a point there ^^

Copy link
Collaborator

@shellscape shellscape left a comment

Choose a reason for hiding this comment

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

We'll need a test for this change before we can merge it. I know it looks small on the surface but this is a breaking change, and we need a test to prevent regressions in the future.

Would also like to get @lukastaegert take on this. There may be an undocumented reason for the existing behavior.

@lukastaegert
Copy link
Member

My understanding is that the resolve default behavior matches what Node does (though I did not check), so it is technically correct. If we change this, then this is a breaking change. Which might be fine if we think this is basically never what people want.

@bytestream
Copy link
Contributor Author

I think it's simplest if the README is updated, to either specify the default is null, or remove the default line altogether. The context for me noticing is something similar to vitejs/vite#6767 - permission denied on docker

@shellscape
Copy link
Collaborator

@lukastaegert ping re: convo

@shellscape
Copy link
Collaborator

@bytestream please resolve that conflict and we can merge

@shellscape
Copy link
Collaborator

@bytestream thanks for raising the fix and for keeping the branch updated. I updated your branch with the proper tests and updated it to upstream. please understand that (nearly) every PR that gets merged has to have a test, including this one. I had some extra time today to add that, but I don't normally, and we don't normally have a lot of maintainers with extra time on this repo (unfortunately, I wish we did). so in a normal cycle, your PR would have sat unmerged until it was eventually marked as abandoned. we'd love to have you contributing more PRs but please do understand that tests are required and essential to keeping a high quality of shipped code for the repo.

@shellscape shellscape merged commit ab3f45d into rollup:master Oct 8, 2023
5 checks passed
@bytestream bytestream deleted the node-resolve-module-paths branch October 9, 2023 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants