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] Regression in 2.20.1 #1643

Closed
South-Paw opened this issue Feb 3, 2020 · 15 comments
Closed

[import/order] Regression in 2.20.1 #1643

South-Paw opened this issue Feb 3, 2020 · 15 comments

Comments

@South-Paw
Copy link

South-Paw commented Feb 3, 2020

Installed 2.20.1, started getting ordering errors like the following;

image

Our rule for import/order is

'import/order': [
  'error',
  {
    groups: ['builtin', 'external', 'parent', 'sibling', 'index'],
    'newlines-between': 'never',
  },
],

Rolling back eslint-plugin-import to 2.20.0 resolves the issue.

@South-Paw South-Paw changed the title [import/order] regression in 2.20.1 with Typescript [import/order] Regression in 2.20.1 Feb 3, 2020
@yoyo837
Copy link

yoyo837 commented Feb 3, 2020

Seems to be seen only on Windows. 🤔

tylerbutler added a commit to microsoft/FluidFramework that referenced this issue Feb 3, 2020
This is a workaround to a regression introduced in v2.20.1:

import-js/eslint-plugin-import#1643
@ljharb
Copy link
Member

ljharb commented Feb 4, 2020

That makes testing for it difficult; appveyor tests are currently failing for unrelated reasons.

@targos
Copy link

targos commented Feb 4, 2020

What I found by debugging:

  • The import type of an external module is not determined as such
  • The reason is that the path passed to isSubpath contains Windows separators (backslashes) and the function expects slashes.

https://github.com/benmosher/eslint-plugin-import/blob/45f08609e0dd79f2a061c3411a43169c20e80d3a/src/core/importType.js#L31-L41

@ljharb
Copy link
Member

ljharb commented Feb 4, 2020

path.normalize and path.sep might help us here?

@kryops
Copy link

kryops commented Feb 4, 2020

This also seems to affect import/no-extraneous-dependencies: As the import type is reported as internal instead of external due to the path containing backslashes, the rule never reports any errors on Windows.

@kryops
Copy link

kryops commented Feb 4, 2020

I tried to create a minimal reproduction for the import/order and import/no-extraneous-dependencies rules: https://github.com/kryops/eslint-plugin-import-issue1643

@ljharb
Copy link
Member

ljharb commented Feb 4, 2020

A PR with a failing test case would be most welcome; I can add a fix to that (altho a PR that has both is even better!)

@targos
Copy link

targos commented Feb 4, 2020

I think the existing tests should already fail on Windows. I was unable to run the tests on that platform when I tried today, though.

darekkay added a commit to darekkay/dashboard that referenced this issue Feb 4, 2020
curtisman added a commit to microsoft/FluidFramework that referenced this issue Feb 4, 2020
…ch (#1139)

Also pin eslint-plugin-import to v2.20.0

This is a workaround to a regression introduced in v2.20.1:

import-js/eslint-plugin-import#1643
curtisman added a commit to microsoft/FluidFramework that referenced this issue Feb 4, 2020
…ease/0.13 branch (#1139)

Also pin eslint-plugin-import to v2.20.0

This is a workaround to a regression introduced in v2.20.1:

import-js/eslint-plugin-import#1643
@raphinesse
Copy link
Contributor

I had to roll back to 2.19.1 for this issue to go away. 2.20.0 complained about the order of the following imports:

const nearley = require('nearley');
const grammar = require('./grammar');

@ljharb
Copy link
Member

ljharb commented Feb 17, 2020

@South-Paw can you check on latest master of this plugin and see if it's fixed?

@ljharb
Copy link
Member

ljharb commented Feb 18, 2020

I'll close this for now, but am happy to reopen if the issue isn't solved on latest master.

@ljharb ljharb closed this as completed Feb 18, 2020
yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this issue Feb 27, 2020
@thyde1
Copy link

thyde1 commented Mar 11, 2020

If this issue has been resolved in master, is there a reason that it has not yet been released?

@ljharb
Copy link
Member

ljharb commented Mar 11, 2020

Because maintainers are humans who aren’t being paid for their time, and can’t always drop everything to cut a release :-)

yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this issue Mar 26, 2020
yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this issue Mar 26, 2020
sergiohgz added a commit to verdaccio/monorepo that referenced this issue Mar 26, 2020
There is a regression not released yet in 2.20.1 that breaks import/order rule in Windows. See more [here](import-js/eslint-plugin-import#1643)
sergiohgz added a commit to verdaccio/monorepo that referenced this issue Mar 27, 2020
There is a regression not released yet in 2.20.1 that breaks import/order rule in Windows. See more [here](import-js/eslint-plugin-import#1643)
sergiohgz added a commit to verdaccio/monorepo that referenced this issue Mar 28, 2020
There is a regression not released yet in 2.20.1 that breaks import/order rule in Windows. See more [here](import-js/eslint-plugin-import#1643)
@rmlevangelio
Copy link

Hi collaborators, thanks for great plugin. I just want to say that it still happening on Windows.

@ljharb
Copy link
Member

ljharb commented Nov 3, 2020

@rmlevangelio thanks for the report - if you could file a new issue with more details that would be appreciated.

reminjp added a commit to reminjp/css-animation that referenced this issue Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

8 participants