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

TypeScript import/order regression #1525

Closed
joaovieira opened this issue Oct 29, 2019 · 8 comments · Fixed by #1526
Closed

TypeScript import/order regression #1525

joaovieira opened this issue Oct 29, 2019 · 8 comments · Fixed by #1526

Comments

@joaovieira
Copy link
Contributor

I've just upgraded to eslint-plugin-import@2.18.2 and eslint-import-resolver-typescript@2.0.0 and started to get import/order errors:

Screen Shot 2019-10-29 at 17 28 05

Any module defined in @types/* (e.g. @types/express in this case) is seen as "internal", while modules with their own types (e.g. http-status-codes) are seen as "external". I've tried with other modules as well.

Changing the default order to:

    'import/order': [
      'error',
      {
        groups: ['internal', 'external'],
      },
    ],

Gets rid of the error.

Can't understand the root cause though, but here's some logging from the resolver if it helps:
Screen Shot 2019-10-29 at 22 09 36

The import/order rule history doesn't seem to have anything suspicious. Could the new version of the resolver be causing this?

Re: import-js/eslint-import-resolver-typescript#33

@joaovieira
Copy link
Contributor Author

joaovieira commented Oct 29, 2019

And this is the resolver log with eslint-import-resolver-typescript@1.1.1:

Screen Shot 2019-10-29 at 22 39 34

As you see, the new version resolves the correct definition under node_modules/@types/**/*.d.ts, while the old one resolved the original node_modules/**/*.js.

@ljharb
Copy link
Member

ljharb commented Oct 29, 2019

Yes, that sounds like it's due to the typescript resolver.

@joaovieira
Copy link
Contributor Author

And the last evidence 🙂:

Screen Shot 2019-10-29 at 22 54 33

Screen Shot 2019-10-29 at 22 52 14

Will look at the code now.

@joaovieira
Copy link
Contributor Author

So, the package name is express, path is node_modules/@types/express/index.d.ts:

  • it passes the external regexp
  • but it's not external because path.indexOf('node_modules/express') == -1

We could simply use the import/external-module-folders setting to include the @types folder as ['node_modules', 'node_modules/@types'] in the typescript config, or change the isExternalPath logic to account for these scenarios.

@ljharb what do you think is best? I'm worried that with the setting it doesn't solve the problem for people that are already overriding it.

@Evalon
Copy link

Evalon commented Oct 30, 2019

Answer from eslint-import-resolver-typescript:
import/order generates wrong order with 2.0

@South-Paw
Copy link

South-Paw commented Feb 3, 2020

I think I've also got this issue in our projects at work when updating things...

image

@Evalon's linked issue has been closed and locked and now @ljharb has closed this issue without an explanation or mention of it being explicitly fixed? 🤔

So what's actually the suggested solution here? Are we waiting on something to be merged or does something else need doing somewhere?

Edit: realize now this issue might be unrelated due to it's age and rolling back to 2.20.0 resolved my issue - created: #1643

@ljharb
Copy link
Member

ljharb commented Feb 3, 2020

@South-Paw in fact, it was closed by the PR #1526, which has been released - do you not see that in the timeline entry?

@South-Paw
Copy link

South-Paw commented Feb 3, 2020

@ljharb sorry that one is definitely on me as I totally missed that 🤦‍♂

I think the issue I'm seeing is unrelated to this one though, so sorry for digging up the wrong issue to reply to 😟 - I've created #1643

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.

4 participants