-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fixes #954 - strip out webpack loaders for no-extraneous-dependencies #987
fixes #954 - strip out webpack loaders for no-extraneous-dependencies #987
Conversation
@@ -78,6 +79,13 @@ ruleTester.run('no-extraneous-dependencies', rule, { | |||
message: '\'not-a-dependency\' should be listed in the project\'s dependencies. Run \'npm i -S not-a-dependency\' to add it', | |||
}], | |||
}), | |||
test({ | |||
code: 'import "raw-loader!not-a-dependency"', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can’t this syntax take any value? I don’t think it can be generically handled; i think each loader needs it’s own custom resolver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, could you explain what you mean? I thought this would be a generic solution. It should work with every loader or multiple ones and nothing needs to be resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean is; import "some-loader!not-a-package-name-or-path"
would work too. There's no guarantee that the thing after the !
is a normal require.resolve
-able path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I think I understand you. But isn't this always the case? Or at least not specific to my PR?
But maybe I just don't understand eslint-plugin-import
good enough. Is there something I should do differently in my PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I'm suggesting is that your entire approach is flawed; it's based on the idea that if you take the part after the !
, then you can resolve it normally, as if the first part wasn't there.
However, that's just not how webpack's special ! syntax works. Which means that every single custom loader syntax needs its own snowflake package to tell eslint-plugin-import
how to resolve it - there's no way that the core plugin itself can handle it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. The part behind the loader might not be a package at all, and that should not be an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry. I don't understand what is wrong 😞
When I write import "foo"
I never know if foo
is a package or not. What is the difference between import "foo"
and import "bar!foo"
from the point of view of the no-extraneous-dependencies
? AFAIK it should just ignore the bar!
part.
Is your objection against stripping the loader part at all or just against the test...? 🤔 Still try to wrap my head around this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you write import 'foo'
, you do know, with certainty, that foo
is an import path - eslint-plugin-import
has a "node" resolver, for example, that uses node's rules to resolve that path. import 'bar!foo'
in the "node" resolver would look for a file literally named bar!foo.js
or bar!foo/index.js
etc.
Thus, in order to have !
have different rules, an entirely distinct resolver package would need to be created to tell eslint-plugin-import how to resolve them - that knowledge does not belong hardcoded in a core rule.
Separately, you'd want to make a separate resolver for every loader. foo-loader and bar-loader might have entirely and wildly different resolving rules. One loader might make foo!bar
load "bar.js" from "foo.com", let's say. Thus, it's impossible to have any sort of generic way to handle "strings with exclamation points", because there is not a single universal meaning for them, even within webpack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right that !
as part of a file name/directory name is not possible in webpack, but could be possible in non-webpack projects - and this project is not webpack specific. Maybe I'll create s custom rule which works like this rule, but with webpack in mind.
Feel free to close.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need a custom rule so much as a custom eslint-plugin-import resolver plugin.
Hi everyone!
A small attempt to fix #954. I'm not that familiar with eslint-plugin-import so I wasn't sure how "resolvers" should help here or not (#954 (comment)).
However this could be an intermediate solution.