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 behavior with parent and sibling #2685

Open
JohnnyDevNull opened this issue Jan 19, 2023 · 18 comments · May be fixed by #2721
Open

[import/order] Regression in behavior with parent and sibling #2685

JohnnyDevNull opened this issue Jan 19, 2023 · 18 comments · May be fixed by #2721

Comments

@JohnnyDevNull
Copy link

Since Version 2.27.0 we've recognized a change in over 500 files with the ordering rule:

        "import/order": [
          "error",
          {
            "groups": ["builtin", "external", "parent", "sibling", "index"],
            "newlines-between": "always",
            "alphabetize": {
              "order": "asc",
              "caseInsensitive": true
            }
          }
        ],

For example:

@my-local-package/mysub-path
@my-local-package/mysub/path

They are now handled different than before and leads to hundres of linting errors where no before.

@JohnnyDevNull
Copy link
Author

This eventually relates to #2683 and #2682

@ljharb
Copy link
Member

ljharb commented Jan 19, 2023

cc @Pearce-Ropion

@Pearce-Ropion
Copy link
Contributor

According to the 347d78b change, this is not a bug. The change was made to specifically address this specific use case (as far as I understand it)

@ljharb
Copy link
Member

ljharb commented Jan 20, 2023

@JohnnyDevNull how many of those changes are fixable with the autofixer?

@fvsch
Copy link

fvsch commented Jan 29, 2023

We use a similar configuration to @JohnnyDevNull in a large codebase and the import-order rule considered this order valid in 2.26:

import abc from '../../abc'
import def from '../def'
import ghi from './ghi'
import jkl from './jkl'

But in 2.27 it's forcing the following order, which doesn't make sense to us:

import ghi from './ghi'
import jkl from './jkl'
import abc from '../../abc'
import def from '../def'

It looks like our only workaround right now would be to change our config to put "parent" and "sibling" into different groups, forcing adding a blank line between parent and sibling imports.

This regression does seem to come from this change:

According to the 347d78b change, this is not a bug. The change was made to specifically address this specific use case (as far as I understand it)

Looking at #2396, it looks like the intent was to sort forward slashes (/) before hyphens (-), so that alphabetical order would be:

something
something/subpath
something-else

But the change also triggered sorting the / character before dots (.), resulting in the issues reported here for parent and sibling imports in the same alphabetized group.

So the importsSorter function should probably be expanded on a bit to sort dots before slashes, hopefully fixing this while still passing the tests from #2396.

@tjx666

This comment was marked as duplicate.

tjx666 pushed a commit to tjx666/eslint-config that referenced this issue Jan 31, 2023
@Pearce-Ropion
Copy link
Contributor

Quick progress update, I have a fix for this working locally. I'm working on adding a few more unit test cases to maintain code coverage. I should have a PR up by the end of the week.

@notaphplover
Copy link

According to the 347d78b change, this is not a bug. The change was made to specifically address this specific use case (as far as I understand it)

I would hazard to say it's a bug since the expected behavior, as described in the docs is not the actual one.

@jefflage
Copy link

Quick progress update, I have a fix for this working locally. I'm working on adding a few more unit test cases to maintain code coverage. I should have a PR up by the end of the week.

@Pearce-Ropion How's this coming along?

@Pearce-Ropion
Copy link
Contributor

Quick progress update, I have a fix for this working locally. I'm working on adding a few more unit test cases to maintain code coverage. I should have a PR up by the end of the week.

@Pearce-Ropion How's this coming along?

I've put up #2721. I think it needs more unit testing but figured I'd get the PR out there

@tjx666
Copy link

tjx666 commented May 8, 2023

Seems many people stay at 2.26.0 because of this issue:
Screenshot_2023-05-09-03-54-26-944-edit_com mmbox xbrowser

@JohnnyDevNull
Copy link
Author

@JohnnyDevNull how many of those changes are fixable with the autofixer?

It takes too long to run. We have round about 300 libraries in a NX Monorepo. It's impossible to get this through a code review without having new merge conflicts within a week. The only solution is to keep the local path as before or uninstalling the plugin sadly to say.

So we stay at 2.26.0 until it's fixed or we have to remove it because of eslint version upgrades.

@TheSpyder
Copy link

We're also downgrading to 2.26.0 in TinyMCE to avoid this issue, the PR that resulted was too much
tinymce/tinymce@841176b

@JohnnyDevNull

This comment was marked as spam.

1 similar comment
@JohnnyDevNull

This comment was marked as spam.

@tjx666
Copy link

tjx666 commented Aug 23, 2023

for anyone can't wait, recommend: https://github.com/lydell/eslint-plugin-simple-import-sort

@JohnnyDevNull

This comment was marked as spam.

1 similar comment
@JohnnyDevNull

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
8 participants