import-js / eslint-plugin-import

ESLint plugin with rules that help validate proper imports.
MIT License
5.38k stars 1.54k forks source link

Proposal: Allow reading custom "<something>Dependencies" fields in no-extraneous-dependencies #1592

Open obweger opened 4 years ago

obweger commented 4 years ago

Version

2.19.1

Context and problem

In our monorepo, we're exploring a custom resolution strategy to deal with package-by-package dependency upgrades in a controlled way (e.g., only allow exactly two versions of a dependency in the repo, etc.). TLDR, we're using a custom legacyDependencies field in our package.jsons to declare dependencies to "outdated" versions that are supposed to be replaced by the new ones:

  "dependencies": {
    "foo": "^2"
  },
  "legacyDependencies": {
    "bar": "^1"
  },

In the code, you can still import from bar; it's just resolved to a different node_modules:

import foo from 'foo';
import bar from 'bar';

This unfortunately breaks the no-extraneous-dependencies rule, as it doesn't know to check the legacyDependencies field.

Proposed solution

We could extend the existing configuration to not only allow devDependencies, optionalDependencies, peerDependencies, and bundledDependencies, but arbitrary keys - in our case, this would be legacyDependencies. All other parts of the configuration can remain the same, i.e. we could support true/false and the array of globs as we do today.

Are you willing to submit a pull request to implement this change?

Yes.

ljharb commented 4 years ago

Is there a reason you don't repeat the deps? meaning, put it in both deps and legacyDeps, just with different version ranges?

obweger commented 4 years ago

@ljharb great thinking! Technically we could, but I feel it would be confusing to devs (there are 100s of contributors to the monorepo): They would declare something in "dependencies" with version x, but it would magically resolve to the "legacyDependencies" version y.

ljharb commented 4 years ago

Perhaps, but if you're inventing a nonstandard deps field I think confusion is unavoidable and documentation is necessary.

obweger commented 4 years ago

Full agree that documentation will be necessary here!

@ljharb, to move this forward, is this a change the team would be supportive of? It would be trivial to write a custom rule for that in our repo, I'd just strongly prefer to contribute back to the de-facto standard

An alternative, even more flexible solution would be to introduce a configurable "package.json interpreter" (similar to the existing resolver system) that would receive the content of package.json and return all dependencies.

In general, I believe that eslint-plugin-import can only benefit from this flexibility while the industry is still consolidating on standard monorepo setups.

adevnadia commented 4 years ago

Perhaps, but if you're inventing a nonstandard deps field I think confusion is unavoidable and documentation is necessary.

Agree with this, good documentation for something like this will be crucial. But, what would be more confusing, this:

{
   dependencies: {
    'button': '1.2.3'
  },
  legacyDependencies: {
    'modal': '1.2.3'
  }
}

or this?

{
   dependencies: {
    'button': '1.2.3',
    'modal': '5.6.7'
  },
  legacyDependencies: {
    'modal': '1.2.3'
  }
}

:) In the first scenario, even if the field is custom for our repo, it's still pretty much self-explanatory, and the meaning of it can be easily inferred from the context, even without reading the docs. In the second one which of the modals is actually used would be impossible to guess without reading everything and understanding how the system works in every detail.

That's the main reason behind the ask.

ljharb commented 4 years ago

Given that npm/yarn's peerDependenciesMeta, and yarn's resolutions feature, all work the second way, I'd say that way is much clearer. Anything that's completely absent from the normal package.json fields won't be read by any ecosystem tooling - adding customization functionality to eslint-plugin-import won't help you there, but following my suggestion seems like it would obviate the problem for all tools.

adevnadia commented 4 years ago

Regardless of implementation details in our repo (there are a few more reasons why we can't just declare deps always), would the proposed change be accepted? Are there any disadvantages to it?

I think that it can be useful not only useful to us, but to the community as well: it would allow all the ecosystem to experiment with non-standard package.json fields, and even roll out experimental features in that area, with eslint fully functional right from the get-go ✌🏽

ljharb commented 4 years ago

To be honest, allowing the community to experiment with nonstandard package.json fields isn’t imo a good thing. node and npm itself cant use any field names that the ecosystem has imposed its own meanings on (just like globals on the web). The community’s use of the “module” field prevented node’s own module implementation from using that name, for example.

I haven’t decided for sure whether to accept this or not - but it is definitely not a clear win, even setting aside the normal complexity tradeoffs of adding any feature.

adevnadia commented 4 years ago

Hi @ljharb, did you do some more thinking about this proposal?

We are about to start the work on our side, would be great to just contribute it to your rule instead of inventing custom solutions just for us.

ljharb commented 4 years ago

I think this really is something best served by custom tooling; the ecosystem overwhelmingly always declares every single dep used in a project in either dependencies, devDependencies, optionalDependencies, or bundleDependencies (peerDependencies would only be for things used by consuming projects).

Additionally, yarn v2, pnpm, and other tools actually break require for things that aren't explicitly listed, so I think the trend is pretty clear.