-
-
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
Consider that some node_modules
folders aren't vendor
#677
Comments
The rules get the dependencies from the nearest package.json file not the node_modules dir |
@HenriBeck yes, I know. But, the problem is that this rule thinks that every module that is resolved by a I'm pointing a case that the For example, in that structure I shown: If in Actually, we can see this problem as a bug. |
I think the problem is here: It's the way that the whole plugin classifies a module as “external”:
It could be:
|
The idea of the rule is to ensure that all dependencies are in a package.json and npm-installable. Nested That said, I'd support an option where you can manually and explicitly exempt certain module names from being checked. |
Sorry if I'm missing something, or I can't express myself enough in english writing. I'm not going against the ideia of the rule (“ensure that all dependencies are in a
Now the case that I put my modules inside a
So the rule still exists, but more reasonable about what is really an external dependency. I think that all the stuff about npm and gitignore comes after. And in my point of view, the abuse is actually considering that |
In your example, |
The goal of the change I proposed is to provide support precisely for people that disagree with these affirmations, for various reasons. And I'm sure that it would change nothing for those that follow the structure of a unique People not always like to do Anyway, I'm okay closing the issue if that's not interesting right now. Thanks for the attention! 😃 |
@ljharb I have to agree @daltones because you self have said that anyone can create a folder in node_modules. IMHO it's a valid option to create a node_modules folder to solve the '../../../' require issue. |
@k15a pointing out that it's possible isn't the same thing as saying that it's a good idea. The solution to that "issue" is either a) just get over it because it's not that bad, or b) extract more things out to separate npm-installed modules. |
But isn't it all what @daltones is trying to point out? That's possible to put code into a node_modules folder isn't a NPM package? If I understand no-extraneous-dependencies correctly it's linting that all npm packages are listed in the package.json. |
Right - I'm saying that the entire point of |
Ok then I did understand the rule wrong. |
I'm going to agree with @ljharb's last statement: this rule (naïvely, but pragmatically) assumes that everything in If that doesn't hold, the rule doesn't match your use case. I would like to keep it simple. Also, FWIW, I occasionally have non-NPM module code in // package.json
{"dependencies": {
"my-cool-shared-lib": "file:../my-cool-shared-lib"
}} vs. actually having the code live in Closing for now. |
I'd like to propose a change in
no-extraneous-dependencies
rule.Currently, that rule understands that every module from a
node_modules
folder is a dependency that should be specified inpackage.json
.The last few days I was considering drop all kinds of hacks to resolve modules in my projects (such as
babel-plugin-module-resolver
or somewebpack
configuration). And the solution is obvious: I could use anode_modules
folder inside mysrc
folder for all my modules.I guess that is the real standard way intended first, but then people started to associate the
node_modules
folder to strictlynpm
dependencies.So I propose that
no-extraneous-dependencies
considers onlynode_modules
folder that are sibling of apackage.json
.Then we could have a structure like this:
Inside
src/
we can reference every module in a absolute way, because we're just following the built in resolving logic of Node. 😃Later I saw (if I understood right) that Create React App is following that idea too (see facebook/create-react-app#1065)
The text was updated successfully, but these errors were encountered: