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

Consider that some node_modules folders aren't vendor #677

Closed
daltones opened this issue Nov 24, 2016 · 13 comments
Closed

Consider that some node_modules folders aren't vendor #677

daltones opened this issue Nov 24, 2016 · 13 comments

Comments

@daltones
Copy link
Contributor

daltones commented Nov 24, 2016

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 in package.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 some webpack configuration). And the solution is obvious: I could use a node_modules folder inside my src 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 strictly npm dependencies.

So I propose that no-extraneous-dependencies considers only node_modules folder that are sibling of a package.json.

Then we could have a structure like this:

project/
  package.json
  node_modules/        # there could be some extraneous dependency
  src/
    node_modules/      # every module inside this folder would be not extraneous, because there's no sibling package.json
      foo.js
      bar.js
      baz/
        index.js
    index.js

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)

@HenriBeck
Copy link

https://github.com/benmosher/eslint-plugin-import/blob/master/src/rules/no-extraneous-dependencies.js#L8

The rules get the dependencies from the nearest package.json file not the node_modules dir

@daltones
Copy link
Contributor Author

daltones commented Nov 24, 2016

@HenriBeck yes, I know. But, the problem is that this rule thinks that every module that is resolved by a node_modules folder is a external dependency that must be specified in package.json.

I'm pointing a case that the node_modules folder is not for externals.

For example, in that structure I shown: If in project/src/index.js I import foo, this rule will warn about foo not being listed in package.json. That's wrong, because that project/src/node_modules folder is not associated with project/package.json (it's not for externals).

Actually, we can see this problem as a bug.

@daltones
Copy link
Contributor Author

I think the problem is here:
https://github.com/benmosher/eslint-plugin-import/blob/master/src/core/importType.js#L21

It's the way that the whole plugin classifies a module as “external”:

  • residing inside a node_modules (or what else is configured).

It could be:

  • residing inside a node_modules (or what else is configured) that is sibling of a package.json.

@ljharb
Copy link
Member

ljharb commented Nov 24, 2016

The idea of the rule is to ensure that all dependencies are in a package.json and npm-installable. node_modules should be gitignored, and npm install --production or npm install used to reconstruct it.

Nested node_modules are a bad idea, despite the fact that the node require algorithm uses them. Abusing that fact to put non-npm-installed modules in node_modules isn't really in the spirit of the rule :-/

That said, I'd support an option where you can manually and explicitly exempt certain module names from being checked.

@daltones
Copy link
Contributor Author

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 package.json”). Let's think this way:

  • I just did npm install lodash (no --save).
  • package.json was not modified and Lodash was placed in node_modules.
  • In my code, I wrote import _ from 'lodash'.
  • lodash was found, but no-extraneous-dependencies complains about missing in package.json
    • Good! That's this rule doing its job!

Now the case that I put my modules inside a src/node_modules/ folder:

  • I have a src/node_modules/foo/bar.js.
  • In some other file I wrote import bar from 'foo/bar'.
  • foo/bar was found, but no-extraneous-dependencies complains about missing in package.json
    • Wait, what package.json? There's no src/package.json, so it's not the case of a external dependency. It's not even a dependency, it's my own code.

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 node_modules folder is by definition a bag of external dependencies. There's more about this way of thinking in those issues of Create React App.

@ljharb
Copy link
Member

ljharb commented Nov 25, 2016

In your example, foo should be npm-installable. Your code - ie, code not npm-installed - belongs outside node_modules and should always be required/imported with a relative path.

@daltones
Copy link
Contributor Author

daltones commented Nov 29, 2016

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 node_modules just for dependencies.

People not always like to do ../../../foo.js, and they try to reach absolute imports very often with all kinds of hacks (babel transforms, custom resolvers, webpack config, symlinks, entirely directory copy, NODE_PATH). But in the end maybe they just wouldn't need… 😅

Anyway, I'm okay closing the issue if that's not interesting right now. Thanks for the attention! 😃

@k15a
Copy link
Contributor

k15a commented Nov 29, 2016

@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.

@ljharb
Copy link
Member

ljharb commented Nov 29, 2016

@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.

@k15a
Copy link
Contributor

k15a commented Nov 29, 2016

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.

@ljharb
Copy link
Member

ljharb commented Nov 29, 2016

Right - I'm saying that the entire point of no-extraneous-dependencies is to ensure that everything you're requiring comes from package.json. If you disagree with this and want to make folders in node_modules, this isn't the rule for you.

@k15a
Copy link
Contributor

k15a commented Nov 29, 2016

Ok then I did understand the rule wrong.

@benmosher
Copy link
Member

I'm going to agree with @ljharb's last statement: this rule (naïvely, but pragmatically) assumes that everything in node_modules is an npm install.

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 node_modules, but it is normally a filesystem dependency to somewhere else:

// package.json
{"dependencies": {
  "my-cool-shared-lib": "file:../my-cool-shared-lib"
}}

vs. actually having the code live in node_modules, and then I use linklocal to create symlinks to the source.

Closing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants