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

import/order fails to recognize internal import groups #807

Closed
yenbekbay opened this issue Apr 20, 2017 · 44 comments
Closed

import/order fails to recognize internal import groups #807

yenbekbay opened this issue Apr 20, 2017 · 44 comments

Comments

@yenbekbay
Copy link

yenbekbay commented Apr 20, 2017

I'm using babel-plugin-module-resolver with eslint-import-resolver-babel-module to map imports from ~/<path> to src/<path>:

In my ESLint config, the import/order rule is turned on using the following configuration:

module.exports = {
  rules: {
    'import/order': [
      2,
      {
        groups: [
          'builtin',
          'external',
          'internal',
          ['parent', 'sibling', 'index'],
        ],
        'newlines-between': 'always',
      },
    ],
  },
};

Imports from ~/<path>, I believe, should be classified as an internal import group separated from other groups by a newline. I'm not sure what the behaviour is here, but given the following code:

import someNpmModule from 'some-npm-module';

import SomeComponent from '~/components/SomeComponent';
import someUtil from '~/utils/someUtil';

I'm receiving the following error:

   3:1   error  There should be at least one empty line between import groups  import/order
@Whoaa512
Copy link

Also seeing this issue, even without the resolver plugin. Would love to see this fixed, or if there's something missing in the docs about how to configure it please let us know.

Whoaa512 added a commit to Whoaa512/eslint-plugin-import that referenced this issue May 1, 2017
Including one broken test related to the import-js#807
Whoaa512 added a commit to Whoaa512/eslint-plugin-import that referenced this issue May 1, 2017
Including one broken test related to the import-js#807
@lo1tuma
Copy link
Contributor

lo1tuma commented May 12, 2017

I think what is missing is an option for custom resolver to define to which group a resolved file belongs to.

@Whoaa512
Copy link

Whoaa512 commented Aug 3, 2017

@lo1tuma Sorry if this is a dumb question but where would I add that?

@lo1tuma
Copy link
Contributor

lo1tuma commented Aug 3, 2017

@Whoaa512 AFAIK that’s not yet implemented in eslint-plugin-import.

@Whoaa512
Copy link

Whoaa512 commented Aug 3, 2017

Right, I was asking were I to contribute to eslint-plugin-import where in the code would I add support for custom resolvers?

@yenbekbay
Copy link
Author

The root cause of the issue seems to be the fact that the import type of a path like ~/path/to/some/file.js is resolved as unknown. This happens because the /^\w/ regular expression doesn't cover the tilde symbol.

@ljharb
Copy link
Member

ljharb commented Aug 5, 2017

How does node handle that? ~ as the user's home dir isn't exactly a cross-platform thing.

@Whoaa512
Copy link

Whoaa512 commented Aug 8, 2017

Edit: Actually that proposed solution doesn't solve the issue so please ignore

tl;dr: Can we change /^\w/ to /^[^\.]/

@ljharb, Node handles the os.homedir() call using a C++ binding to the operating system. However this particular issue isn't related to that.

This issue deals with the fact that the OP has configured his babel setup to replace instances of ~/ in require/import statements with the root of his project (I presume).

Now I think the discussion needs to take a turn since we've come to the cause of the issue, as noted by @yenbekbay (#807 (comment)).

Basically can we change the regex for determining if import name is an external module, from /^\w/ to something more flexible that would allow for the leading ~.

I would propose changing it to /^[^\.]/, which would mean that any non-relative path would be considered external. I think that is the intended behavior but I'm open to discussion about why that might not be the case.

@ljharb
Copy link
Member

ljharb commented Aug 9, 2017

What about absolute paths?

@Whoaa512
Copy link

Whoaa512 commented Aug 9, 2017

Maybe the actual fix is just to allow a setting which gives flexibility to what is considered an internal module.

Maybe something like this:

function isInternalModule(name, settings, path) {
  const folders = settings && settings['import/internal-module-folders']
  const customInternalModule = folders && folders.some(folder => name.startsWith(folder))
  return customInternalModule || externalModuleRegExp.test(name) && !isExternalPath(path, name, settings)
}

@stramel
Copy link

stramel commented May 14, 2018

Any resolution on this? I just ran into the same issue. it won't even show warnings for me with ~/ using eslint-import-resolver-babel-module

@Whoaa512
Copy link

@stramel There's a PR (#914) to address this, but it needs changes which I'm unable to commit to doing at this time.

If you like I could add you as a collaborator on my fork.

@stramel
Copy link

stramel commented May 15, 2018

@Whoaa512 I can give it a shot this weekend.

@loopmode
Copy link

FYI, my use-case is a yarn workspace with namespaced packages.
So, I have several packages in packages/, all of them are prefixed with a namespace like @x-client/ or @x-server/.
Using yarn workspace, I can just import like that, e.g. import { getBaseUrl } from '@x-client/api'; without relative paths.

When using import/order, all my actually internal packages are treated as external, and it would be great to have some control over that treatment.

@domhhv
Copy link

domhhv commented Apr 16, 2019

Hi, I just ran into a similar issue in our Angular project.

My rule is configured as the following:

"import/order": [
  "error",
  {
    "groups": ["builtin", "external", "internal", "sibling", "parent"],
    "newlines-between": "always"
  }
]

Code raising an issue:

import { NavigationEnd, NavigationStart, Router } from '@angular/router'; // eslint-disable-line import/order

import { SplashScreenService } from 'shared/splash-screen/splash-screen.service';

If I remove // eslint-disable-line import/order than ESlint complains about newline. It recognizes both @angular/router and shared/splash-screen/splash-screen.service as external modules although the latter is clearly an internal module. And these issues are raising all across our project.

Maybe there should be a way to tell ESlint how to recognize an internal module. Is there a way to fix it or contribute?

@Whoaa512
Copy link

@dgrishajev Feel free to pick up the PR #914, I'll add you as a collaborator on my fork. So you can update the branch related to that PR.

@loopmode
Copy link

loopmode commented Apr 16, 2019

Maybe a bit off topic but.. Can't we just configure certain top-level paths to be explicitly treated as internal, like shared/? I guess given all possibilities of aliasing and module resolving, it would be much easier that way than trying to determine everything heuristically..
Also, using yarn workspaces and lerna, I started to use use namespaces like "@client/" or "@server/", but I might also introduce some namespace that actually exists on the public registry, but is a working local package nonetheless.

@domhhv
Copy link

domhhv commented Apr 17, 2019

@Whoaa512 Thanks, I'll do some research in the next few weeks.

@Whoaa512
Copy link

@loopmode If you wish to use a config to specify top-level internal modules you can use my fork: @fictiv/eslint-plugin-import

Then in your settings in your .eslintrc add the following

settings:
    import/internal-module-folders: ['shared/', '@client/']

@viniciusffj
Copy link

Is there any work happening on this topic? I could put some effort as I am experiencing this issue

Whoaa512 added a commit to Whoaa512/eslint-plugin-import that referenced this issue Jul 9, 2019
@mrmckeb
Copy link
Contributor

mrmckeb commented Sep 24, 2019

@Whoaa512 could you create a PR to add the fix here?

@Whoaa512
Copy link

@mrmckeb Already did but it was closed in favor of implementing a custom resolver option described here #807 (comment)

Unfortunately I'm not able to work on this for the next month or so. Would love a fresh set of eyes to take a stab at it though.

@viniciusffj
Copy link

@Whoaa512 I could try to take a look on it. Could give some initial guidance/context?
Thank you

@mrmckeb
Copy link
Contributor

mrmckeb commented Sep 30, 2019

It might be nice to allow people to just set a list of modules that are internal as a workaround?

@Whoaa512 that sounds good - @viniciusffj I'll be interested to see what you come up with!

@smdern
Copy link

smdern commented Oct 15, 2019

@mrmckeb
Copy link
Contributor

mrmckeb commented Oct 16, 2019

I saw that too @smdern - and I think it would do the trick, however I think the idea of layering resolvers to solve this is not ideal though... and a potential source of issues in future.

@markerikson
Copy link

@smdern : can you give an example of how that alias resolver fixes the issue, and how you would set it up?

@loopmode
Copy link

Yeah, I'd like to know that too.
Is the idea to micro-manage imports via aliases to gain some control over the sorting order??

@markerikson
Copy link

I was able to add the alias resolver, and it does seem to improve the behavior.

In my case, I don't have specific aliases set up, but I do have "absolute imports" set up for a Create-React-App + TS project. My tsconfig.json has:

  "include": [
    "src"
  ]

For my app specifically, the source folder structure is:

- /src
  - /app
  - /common
  - /features

and so typical imports might look like:

import {RootState} from "app/reducers/rootReducer";
import {showDialog} from "common/dialogs/dialogsRegistry";
import {logout} from "features/auth/authSlice";

Those are imports I would want classified as "internal".

I added these settings to my ESLint config:

    "settings": {
        "import/resolver": {
            "typescript": {
                "directory": "."
            },
            "alias": {
                "map": [
                    ["app", "./src/app"],
                    ["common", "./src/common"],
                    ["features", "./src/features"]
                ],
                "extensions": [".ts", ".js", ".jsx", ".json"]
            }
        }
    },
    "rules": {
        "import/order": [
            "warn",
            {
                "groups": ["builtin", "external", "internal", "parent", "sibling", "index"],
                "newlines-between": "always-and-inside-groups"
            }
        ]
    }

That appears to have them mostly sorting how I want - after 3rd-party libs, and before local relative imports.

@smdern
Copy link

smdern commented Oct 22, 2019

I initially tried the typescript resolver, but it kept counting modules that were included via a @types/_whatever package as an internal modules, which messed up the grouping. I setup the alias map just like you did.

If you're using .tsx files be sure to include that in the extensions.

Heres my config
tsconfig.json

{
  "compilerOptions": {
    "paths": {
      "src/*": ["src/*"]
    }
  }
}

.eslintrc.js

const root = (...args) => path.resolve(__dirname, ...args)
{
      settings: {
        'import/resolver': {
          alias: {
            map: [['src', root('client/src')]],
            extensions: ['.ts', '.tsx', '.js', '.jsx', '.json'],
          },
        },
      },
    },
}

@peoplenarthax
Copy link

peoplenarthax commented Dec 3, 2019

@smdern we face the same problem and just adding the setting with "import/external-module-folders": ["node_modules", "node_modules/@types"] a more final answer will be to can detect if it is a typescript parser, but this is a good workaround

@mrmckeb
Copy link
Contributor

mrmckeb commented Dec 4, 2019

Hi there, I resolved this issue the following way. I hope this can help someone.

For JavaScript:

settings: {
  'import/resolver': {
    node: {
      paths: ['./src'],
    },
  },
},

And overrides for TypeScript files:

settings: {
  'import/parsers': {
    '@typescript-eslint/parser': ['.ts', '.tsx'],
  },
  'import/resolver': {
    node: {},
    ts: {
      directory: tsconfigPath,
    },
  },
},

@ra50
Copy link

ra50 commented May 13, 2020

Is this a bug or expected behavior? After reading all these comments and the docs I'm still unclear how to get this plugin to separate external from internal imports. Why aren't files under /src considered internal by default?

@ljharb
Copy link
Member

ljharb commented Jun 5, 2020

@ra50 relative types always parent or sibling; internal is nothing by default. This is expected (and documented).

This is a long thread and I'm not clear on what's actionable here.

What would help the most is a PR with failing test cases (and even better, also a fix! but i can handle that part) so it's clear what needs fixing.

@loopmode
Copy link

loopmode commented Jun 6, 2020

internal is nothing by default. This is expected (and documented).

This is a long thread and I'm not clear on what's actionable here.

@ljharb I believe that missing distinction is what is actionable here: most of us are asking for a new feature and concept of local workspace packages vs registry-installed packages. It doesn't help that no such concept exists and is documented or not.

In the end, from a project's codebase perspective this is a major distinction between "dependencies" and actual "source code".

In modern projects with workspaces, you just can't tell the difference by looking at an imported package name, e.g. import utils from 'utils' could be either a utils package from npm, or a local one.

By the way, workspaces are not just a yarn feature anymore - npm 7 embraces and introduces the exact same concept of workspaces and local packages, see https://github.com/npm/rfcs/blob/latest/accepted/0026-workspaces.md

So what's needed is a conceptual decision for the plugin first: support local packages and allow some control over the handling, or not.

The implementation could be anything from a simple yet good solution where users configure the plugin and tell it "what is local", to more complex solutions that use the actual npm/yarn APIs and inherently know which packages are from local workspaces and which aren't.

@ljharb
Copy link
Member

ljharb commented Jun 7, 2020

I think it would be quite reasonable, once npm 7 is finalized, to have an automatic import group derived from the "workspaces" field in package.json. Would that meet the use case?

@loopmode
Copy link

loopmode commented Jun 7, 2020

Unfortunately, I drove the discussion off-topic.. Workspaces are a different use case entirely. Totally worth supporting, but I don't see how that relates to the OP. Apologies.

@ljharb
Copy link
Member

ljharb commented Jun 7, 2020

In that case, I'll repeat:

I'm not clear on what's actionable here.

What would help the most is a PR with failing test cases (and even better, also a fix! but i can handle that part) so it's clear what needs fixing.

At this point, I'm going to close the thread, but will immediately reopen it once it becomes clear what the remaining issue is :-)

@ljharb ljharb closed this as completed Jun 7, 2020
@ra50
Copy link

ra50 commented Jun 8, 2020

@ljharb Thanks for the reply. I think the confusion here (at least for me) is that people might expect internal to mean "local to the project" vs. "from npm". Can you point me to the documentation that says internal defaults to nothing?

I understand that relative paths will always be parent or sibling. In my case, I'm using absolute paths, not relative paths, to refer to local project files using TypeScipt's baseUrl setting. I want external imports to come before internal imports, where external means "from npm" and internal means "local to my project" (under src). I was able to achieve this using something similar to @mrmckeb's answer, adding this to my .eslintrc.js:

settings: {
   'import/parsers': {
     '@typescript-eslint/parser': ['.ts', '.tsx']
   },
   'import/resolver': {
     typescript: {
       alwaysTryTypes: true
     }
   }
 }

and explicitly specifying the import order using the groups array. With this setup, the plugin treats internal to be my local files under src and it sorts the way I want.

I don't understand why this works though, and it'd be great if the plugin supported this use case out of the box or without such complicated configuration.

@mrmckeb
Copy link
Contributor

mrmckeb commented Jun 8, 2020

I want to clarify the approach I highlighted above also worked within a monorepo, with a root ESLint config.

However, for us, internal means inside a package, and that may be a distinction. We don't count other packages in the monorepo as internal.

I used the ts resolver over the typescript resolver, but it solved all problems and correctly identified internal modules. However, I haven't updated this config since that time and the situation may have changed.

On a new project I'm working on, which is not a monorepo, I just used the below and internal groups are working as expected:

module.exports = {
  extends: [
    // ...
    'plugin:import/typescript',
  ],
  // ...
  settings: {
    'import/resolver': {
      node: {
        paths: 'src',
      },
    },
  },
};

@ljharb
Copy link
Member

ljharb commented Jun 8, 2020

@ra50

From the docs:

// 3. "internal" modules
// (if you have configured your path or webpack to handle your internal paths differently)

The description of "sibling" and "parent" also precludes anything from being left to be classified as "internal".

Note that https://github.com/benmosher/eslint-plugin-import/blob/master/README.md#importinternal-regex can be used to force things to be treated as "internal".

@ra50
Copy link

ra50 commented Jun 12, 2020

@ljharb Thanks. I guess that wasn't clear to me, that statement didn't seem to imply that internal is nothing by default. Also, it's true that any file within the project is technically a sibling or parent, but I thought that was defined by the use of the relative path, and I'm using an absolute path.

import/internal-regex doesn't work for me because I'd have to list every subfolder under src/ for the regex to pick it up. For example if my paths are src/routes/user and src/models/user, my imports are routes/user and models/user and there's no common prefix to use as the regex.

The solution I posted above solves my use case and treats any import within the monorepo as internal, I just don't understand how it's working. @mrmckeb's last response also works if you want monorepo imports outside of the local project to be treated as external instead of internal.

ljharb pushed a commit to Whoaa512/eslint-plugin-import that referenced this issue Jan 31, 2021
Including one broken test related to the import-js#807
ljharb pushed a commit to Whoaa512/eslint-plugin-import that referenced this issue Jan 31, 2021
Including one broken test related to the import-js#807
ljharb pushed a commit to Whoaa512/eslint-plugin-import that referenced this issue Nov 17, 2021
Including one broken test related to the import-js#807
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.