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

Don't respect config setting if in a node_module #994

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Billy-
Copy link

@Billy- Billy- commented Jan 5, 2018

Fixes #666.

This has made the resolve.moduleDirectories finds a node module test to fail - it was the creation of the package.json in the fake node_module that prompted it. I have spent quite some time looking at this test alone already, and it appears that resolveSync is not finding it, and I cannot for the life of me work out why this happens...

All I can say is that I have verified that it's not my code that caused it (checkout master, create package.json in some-module, then run tests), just the creation of the package.json (which, of course, is expected of an npm module..)

It is 3:20AM now for me, think I'm going to call it a night. 😴

@coveralls
Copy link

coveralls commented Jan 5, 2018

Coverage Status

Coverage increased (+0.002%) to 96.212% when pulling 415b002 on Billy-:fix-666 into 359a200 on benmosher:master.

@coveralls
Copy link

coveralls commented Jan 5, 2018

Coverage Status

Coverage increased (+0.002%) to 96.212% when pulling 8293c1e on Billy-:fix-666 into 359a200 on benmosher:master.

, packageDir

log('Config path from settings:', configPath)
// Only respect webpackConfig settings if not in a node_module
if (!~file.indexOf('/node_modules/')) {
Copy link
Member

Choose a reason for hiding this comment

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

please never use !~, that's far too clever for its own good - explicitly compare to a number instead, since that's what indexOf returns.

Copy link
Member

Choose a reason for hiding this comment

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

that is a pretty neat combo operator, but I agree with @ljharb 😁

, packageDir

log('Config path from settings:', configPath)
// Only respect webpackConfig settings if not in a node_module
Copy link
Member

Choose a reason for hiding this comment

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

i'm not sure why this is something we'd want to do. webpack config applies to every single JS file, potentially, including everything inside a node_modules directory.

Copy link
Author

Choose a reason for hiding this comment

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

That’s true, for rules, etc., but I wonder if it applies to aliases or other resolve configs, too? I would be surpsied since I can’t think why anyone would want to affect the module resolution within node_modules... and if webpack did do that then surely there would be tonnes of problems as a result..?

I’d also argue that since this is an eslint plugin, the user is never interested in node_modules, thus this change does not have an adverse affect?

Copy link
Member

Choose a reason for hiding this comment

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

It applies to everything. Webpack is used all the time to modify the resolution of things within node_modules - Airbnb does it at times.

This eslint plugin checks across files - so it's interested in every JS file that's required/imported, full stop.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, so is the only solution to resolve the path relative to the user’s project? I can’t see any obvious way to do that..

@ljharb ljharb marked this pull request as draft February 1, 2021 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Webpack resolver looks for custom webpack config in packages with jsnext:main
4 participants