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.

Sign up for GitHub

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
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
082663b
Fix relative webpackConfig resolution
Billy- Jan 5, 2018
457342a
Documentation
Billy- Jan 5, 2018
4681285
Update changelog
Billy- Jan 5, 2018
6989a3a
Updates test
Billy- Jan 5, 2018
35970ae
Fix gitignore for resolvers
Billy- Jan 5, 2018
f34cdde
Adds package.json to test module
Billy- Jan 5, 2018
415b002
Grammar/English in comment
Billy- Jan 5, 2018
8293c1e
Update changelog
Billy- Jan 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ build/Release
# https://www.npmjs.org/doc/misc/npm-faq.html#should-i-check-my-node_modules-folder-into-git-
node_modules
!tests/**/node_modules/
!resolvers/**/test/**/node_modules

# Users Environment Variables
.lock-wscript
Expand Down
4 changes: 3 additions & 1 deletion resolvers/webpack/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
This change log adheres to standards from [Keep a CHANGELOG](http://keepachangelog.com).

## Unreleased
- Fix [#666] - only respect config and configIndex settings if not in a node_module ([#994])

## 0.8.3 - 2017-06-23
### Changed
Expand Down Expand Up @@ -87,7 +88,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel
### Added
- `interpret` configs (such as `.babel.js`).
Thanks to [@gausie] for the initial PR ([#164], ages ago! 😅) and [@jquense] for tests ([#278]).

[#994]: https://github.com/benmosher/eslint-plugin-import/pull/994
[#683]: https://github.com/benmosher/eslint-plugin-import/pull/683
[#572]: https://github.com/benmosher/eslint-plugin-import/pull/572
[#569]: https://github.com/benmosher/eslint-plugin-import/pull/569
Expand All @@ -102,6 +103,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel
[#164]: https://github.com/benmosher/eslint-plugin-import/pull/164

[#681]: https://github.com/benmosher/eslint-plugin-import/issues/681
[#666]: https://github.com/benmosher/eslint-plugin-import/issues/666
[#435]: https://github.com/benmosher/eslint-plugin-import/issues/435
[#411]: https://github.com/benmosher/eslint-plugin-import/issues/411
[#357]: https://github.com/benmosher/eslint-plugin-import/issues/357
Expand Down
19 changes: 14 additions & 5 deletions resolvers/webpack/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,18 @@ var log = require('debug')('eslint-plugin-import:resolver:webpack')

exports.interfaceVersion = 2

/**
* Settings object
* @typedef {Object} settings
* @property {string} configPath the path to the webpack config
*/
/**
* Find the full path to 'source', given 'file' as a full reference path.
*
* resolveImport('./foo', '/Users/ben/bar.js') => '/Users/ben/foo.js'
* @param {string} source - the module to resolve; i.e './some-module'
* @param {string} file - the importing file's full path; i.e. '/usr/local/bin/file.js'
* TODO: take options as a third param, with webpack config file name
* @param {settings} settings - settings object
* @return {string?} the resolved path to source, undefined if not resolved, or null
* if resolved to a non-FS resource (i.e. script tag at page load)
*/
Expand All @@ -44,12 +49,16 @@ exports.resolve = function (source, file, settings) {
}

var webpackConfig

var configPath = get(settings, 'config')
, configIndex = get(settings, 'config-index')
, configPath
, configIndex
, 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..

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 😁

configPath = get(settings, 'config')
configIndex = get(settings, 'config-index')
log('Config path from settings:', configPath)
}

// see if we've got a config path, a config object, an array of config objects or a config function
if (!configPath || typeof configPath === 'string') {
Expand Down
12 changes: 12 additions & 0 deletions resolvers/webpack/test/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,4 +91,16 @@ describe("config", function () {
.and.equal(path.join(__dirname, 'files', 'some', 'goofy', 'path', 'foo.js'))
})

it("ignores config setting when resolving inside a dependency", function () {
const npmModulePath = path.resolve(__dirname, 'files', 'node_modules', 'some-module')
const npmModuleFile = path.resolve(npmModulePath, 'foo.js')

// Repeat of test against foo alias, for a control
// (Without this, if the alias was changed, this test would silently regress)
expect(resolve('foo', file, absoluteSettings)).to.have.property('path')
.and.equal(path.join(__dirname, 'files', 'some', 'absolutely', 'goofy', 'path', 'foo.js'))
// Actual test
expect(resolve('foo', npmModuleFile, absoluteSettings)).to.have.property('found')
.and.equal(false)
})
})
Empty file added resolvers/webpack/test/files/node_modules/some-module/package.json
View file
Open in desktop
Empty file.