Rule preventing pulling in a whole package

[dozoisch]

Some repos have defined structure to include just a few files instead of the whole bundle (eg: react-bootstrap, lodash).

We had developed an eslint plugin a while ago to have a rule to prevent this. I don't see the point of having a standalone package just for that rule, and I'd like to add it here instead. (And I haven't seen that rule here)

This is the current repo:
https://github.com/eslint-plugins/eslint-plugin-lean-imports

Let me know if you are interested!

Here is an example:

// with   "lean-imports/import": [1, [ "react-bootstrap "] ]
import { Button } from 'react-bootstrap'; // warns
import Button from 'react-bootstrap/lib/button'; // passes

[benmosher]

I'm up for that. @jfmengels, any thoughts?

I'd rename to import/lean but other than that, makes sense to me.

[jfmengels]

I personally don't like importing that way. To me, everything the lib exports should be available in the main file of the package, and you should just use that, instead of forcing the user to know where stuff is located. That said, when you want to bundle your JS and you don't have tree-shaking or it's not working as you would like it to work, then this can still be a nice rule to have, so 👍. (though I still don't like the reason why we have to do it 😅)

I would be interested in having the opposite rule (or rather, an option that does the opposite) for the reasons I mentioned above.

👍 for import/lean

PS: For Lodash, you have babel-plugin-lodash that does the work for you.

[benmosher]

@jfmengels: isn't https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/no-internal-modules.md the complement of this proposal?

[jfmengels]

Ah yes, my bad.
Well then, we should document in both rules that they are incompatible.

[dozoisch]

Perfect, I'll put up a PR!

[ljharb]

I'm not sure how this rule works - how do you verify that the file-path-based import is === to the named-import-based import? Only if that is true should importing a named import from the top level be warned on.

[dozoisch]

@ljharb This rule is useful for people that don't have tree shaking. Because in that case, importing a named import still pulls the whole package which is not good for your bundle size when you only need 1 file. We currently don't do verification that the path is good. It's just a simple verification not to pull the whole bundle. If people manually require the main index, then :/

[ljharb]

@dozoisch tree-shaking barely works half the time, so I'm strongly in favor of this rule - I'm asking how it's implemented, not why it's desired.

I think that in order for this rule to be added, it would need to, for every named import of the "main" package value, traverse the graph and locate the identical import at its original entry point - and then explicitly recommend the user use that entry point instead of the "main" entry point.

There's too high a danger of confusion if it's simply a rule that bans named imports - some packages named-export values that don't exist in other entry points, and sometimes the named export is a different value than the naming of another entry point might suggest.

[dozoisch]

Currently what it does is very simple. It warns you if you include the main package. It doesn't do any traversing or anything like it.

This is something that could be done as a step 2 to improve the rule though! I would see the step one being just the rule migration as it is.

[ljharb]

I don't think it's a useful rule if it just warns on named imports - that's like a rule that warns on calling a function with > 2 arguments - overly broad and more harmful than useful.

[dozoisch]

It's not just on named imports. It warns any time the main module is imported or required, for a list of modules you specify.

// with "lean-imports/import": [1, [ "react-bootstrap "] ]
import RB from 'react-bootstrap'; // warns
const RB = require('react-bootstrap'); // warns

const Button = require('react-bootstrap/lib/button'); // passes

The goal is really to the prevent require('react-bootstrap') or require('lodash') because it's easy to forget and whoops you just pulled a lot in your bundle. It currently has to be configured to the repos you want manually. Not all repos advocate for the use of direct imports, so we can't really make it for everything by default. But some repos like lodash, react-bootstrap, etc. actually advise you to do and are maintained in a way to keep these files always in the same place, and do proper deprecation before removing them!

I don't see how remembering people not to pull a whole dep is harmful!

[ljharb]

eslint core now has a rule option to handle this (which I PRed in). Use no-restricted-imports and no-restricted-modules, forbid "react", and allow pattern "react/*".

[ljharb]

In other words, there's no benefit in adding it here unless it's doing something new, which would be "follows the dep graph".

[ljharb]

Actually you don't need the pattern at all - you can use both of the no-restricted rules to forbid "react" and it will automatically allow all of the sub-entry points.

[dozoisch]

Oh, I didn't see that rule was there, I can just deprecate for that rule instead! Completely missed it in eslint-core 👍.

It had actually never came to my mind you could use it for that... I'll try to put up a PR on the eslint rule to add this use case in the doc.

[dozoisch]

The error might be a bit more obscure though, since it tells you not to import it at all instead of telling you to pull the files directly.

[ljharb]

Very true - however there's precedent for modifying rules like that to allow for custom error messages. I think that'd be a good issue/PR for eslint core.

[dozoisch]

👍 I'll open an issue in eslint directly, during the day, so it can be discussed in which format it should be done! My only concern is duplication of module names between both rules, but it's a "one time" deal normally, and can be negated by using JS config.

[benmosher]

Closing this since it seems you've decided it's better handled with the core ESLint rule 😎