Incorrect behavior of import/order in a monorepo setup

[skozin]

Minimal reproduction of the bug and a detailed analysis of the cause: https://github.com/skozin/eslint-plugin-import-bug-repro.

The bug

The import/order rule gives a false positive in a setup with the following conditions met:

1. The project uses Yarn workspaces.
2. The packages inside the monorepo are scoped.
3. Webpack resolver is used.

A quick example

Consider this code:

import lodash from 'lodash';
import {sharedFn} from '@my-scope/package-a';

import {someFn} from './util';

export default {};

and this config:

{
  "extends": [
    "plugin:import/errors",
    "plugin:import/warnings"
  ],
  "rules": {
    "import/order": ["error", {"newlines-between": "always"}]
  },
  "settings": {
    "import/resolver": "webpack",
    "import/external-module-folders": ["node_modules", "packages"]
  },
  "parserOptions": {
    "ecmaVersion": 2017,
    "sourceType": "module"
  }
}

The expected result is no errors being reported. However, this comes up instead:

eslint-plugin-import-bug-repro/packages/package-b/index.js
  1:1  error  There should be at least one empty line between import groups        import/order
  4:1  error  `./util` import should occur before import of `@my-scope/package-a`  import/order

I've prepared a repo with a minimal reproduction and a detailed analysis of the cause: https://github.com/skozin/eslint-plugin-import-bug-repro.

What happens

See the repo with a reproduction for a detailed explanation.

In short, the problem is that paths to scoped packages get considered as internal ones by the isExternalPath function even though the directory where these packages reside is added to the import/external-module-folders list in the config. Before #1294, this was not a problem since scoped packages were always considered as external ones, but after merging that PR the behavior changed and now scoped packages are considered external only if their resolved path is determined as external.

In turn, seemingly incorrect behavior of isExternalPath is caused by the logic inside it that doesn't only check that the resolved path to a package contains one of the import/external-module-folders as a subpath, but also that it includes the package name as a subpath. This is not true in the case of a monorepo with scoped packages since a path to a Yarn workspace containing the package contents doesn't need to include the package scope or even the package name as a subpath.

For example, in the described case the resolved path to @my-scope/package-a is /path/to/the/project/packages/package-a/index.js and it doesn't contain @my-scope part at all.

[skozin]

How to fix

This is pretty complicated actually.

The first problem here is that isExternalPath incorrectly determines package name for scoped
packages: it sets the packageName variable by extracting the part of the import name before
the first slash. But this is incorrect for scoped package names since that part corresponds to the
scope name and not package name. For scoped packages, the package name is the part of the import name before the second slash (or the end of the string if the number of slashes is less than two). For example, in our case, for import declaration import {sharedFn} from '@my-scope/package-a' the package name is @my-scope/package-a and not @my-scope.

But even fixing this won't make the plugin support the case described here since Webpack resolver returns the fully resolved path to the module's main file which, in the case of a monorepo, might not contain the scope part or even the package name.

So, I see two options.

The first one is to make isExternalPath return true for all paths that contain one of the
import/external-module-folders as a sub-path, regardless of the name of the import.
According to the documentation, this seems to be the expected behavior:

import/external-module-folders An array of folders. Resolved modules only from those folders will be considered as "external". By default - ["node_modules"]. Makes sense if you have configured your path or webpack to handle your internal paths differently and want to considered modules from some folders, for example bower_components or jspm_modules, as "external".

The risk I see here is that the stuff with checking the import name in isExternalPath was added to work around some edge case I don't yet know of, so removing this seemingly unneeded logic might break something.

The second, safer, way is to add another configuration option, e.g. import/workspace-module-folders, and make isExternalPath return true also when a path to the module contains one of these paths as a sub-path, in addition to the current logic. This will guarantee that none of the existing setups are broken, however at the cost of adding an extra configuration option.

I'm working on a PR implementing the first option but need your input about whether it's safe to do.

[ljharb]

I think that in general, if it doesn't fail tests, it's safe to do, and I definitely prefer the first option to the second.

[skozin]

Thanks, I'll proceed with the first option then, making sure it breaks no existing tests.

[skozin]

The PR is ready: #1605. @ljharb, could you please review it when you have time or suggest someone who could do it?