False positive on Rule "no-import-all-from-library"

[AMorgaut]

Current Behavior

When I import few functions from lodash, the linter raise an issue telling

You not import all from library lodash

Expected Behavior

The code is respecting the rule, so it should not raise an issue here

Steps to Reproduce

1. activate @credengo/eslint-plugin version 2.1
2. add the code import { memoize, omitBy, isNil } from 'lodash'
3. let the linter analyse the code
4. See the raised issues

Failure Logs

> eslint src --fix


/......../src/api/my-code.js
  18:1  warning  You should not import all from library lodash  @creedengo/no-import-all-from-library

creedengo-javascript Version

2.1

SonarQube Version

N/A (directly used from eslint)

[AMorgaut]

I see in the unit tests that they are not considering my use case

valid: [
    `import isEmpty from 'lodash/isEmpty';`,
    `import orderBy from 'lodash/orderBy';`,
    `import { orderBy } from 'lodash-es';`,
    `import map from 'underscore/modules/map.js';,
  ],

https://github.com/green-code-initiative/creedengo-javascript/blob/main/eslint-plugin/tests/lib/rules/no-import-all-from-library.js#L43

Apparently, to comply to the current implementation the given code should be written

import { memoize } from 'lodash'
import { omitBy } from 'lodash'
import { isNil } from 'lodash'

But:

• the rule is still complaining
• it would break the Sonarqube rule Imports from the same module should be merged (javascript:S3863)

[AMorgaut]

Ok so
Looking at the implementation and the history of the file

The implementation works with a default black list which started with "lodash" and "date-fns" and now moved to "lodash" + "underscore" (no jquery or jquery.ui, no momentjs, ....)

    const notAllowedLibraries = ["lodash", "underscore"];
    const importByNamespaceNotAllowedLibraries = ["lodash-es"];

There is an option to manually add other libraries.
It is good, but even developers with good intention won't mostly think to update the eslint configuration when they'll add new libraries to their project

There is a specificity also blacklisting "lodash-es"

    const importByNamespaceNotAllowedLibraries = ["lodash-es"];

In short we can do

    import { foo, bar } from 'lodash-es'

but not

  import * as lodashEs2 from 'lodash-es'

[AMorgaut]

Here is my thinking

The first rule should be to forbid import * as foo from 'foo' every time

It should be true for any library and not just a blacklist. The developer always explicitly tell which functions he needs to import: import { bar, baz } from 'foo'

Pro

• This way, when refactoring his code, or removing features from it, he may realize that the last thing he needs from a library is a short helper he could have internally or get from a lighter library, and finally remove the current library all together
• His code will be shorter and easier to read as he won't have to add foo. in front of all the library functions. Such "namespace" can be long and force the developer to frequently split his code in multiple lines...
• It can help builders to only extract the mandatory code from a library instead including it as a whole

Cons

• It can be appreciated sometimes to differentiate local functions from functions coming from another library. So, using such "namespace" name can help.
• Smart builders could still be able to identify what is really used by the code (but it needs to be tested, at least with webpack and esbuild)

Unless the project has no builder import { memoize } from 'lodash' should be safe

Usually, from this pattern, the builder should not take the unused code (to be verified)

I probably miss some data to realize how much developers, that already work with Sonarqube or at least ESlint, to have a builder on their project.

I personally did such projects with no framework at all. Loving the fact modern browsers natively support import/export. But it means no legacy browser, and then no legacy device, support, which is not green