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

Fix type import ordering #331

Merged
merged 2 commits into from
Dec 9, 2024
Merged

Conversation

rsslldnphy
Copy link
Contributor

@rsslldnphy rsslldnphy commented Dec 6, 2024

it looks like #153 introduced a bug whereby import type ... imports would no longer be sorted with their matching group even if a type-specific group was not present. (see #329 for more details). this pr changes the matching logic such that:

  • non-type imports go in the first matching group if there is one
  • type imports go in the first matching type-specific group if there is one, or the first matching non-type-specific group if there is one
  • otherwise the behaviour remains unchanged, they go in the THIRD_PARTY_TYPES_SPECIAL_WORD or THIRD_PARTY_MODULES_SPECIAL_WORD groups depending on the node type and whether the THIRD_PARTY_TYPES_SPECIAL_WORD exists in the config.

as this behaviour is starting to feel a little complex i've added a comment in the code - i see there's not a ton of comments so happy to take it out if it's against house style. i also don't love the code so refactoring suggestions welcomed.

this fixes #329 - i've tested it on my codebase and the incorrect ordering changes introduced by 5.0.0 were reversed.

@rsslldnphy rsslldnphy changed the title fix type import ordering Fix type import ordering Dec 6, 2024
@byara byara requested review from byara and ayusharma December 8, 2024 12:36
@byara byara added the v5 label Dec 8, 2024
@byara byara changed the base branch from main to v5.2.0 December 8, 2024 12:38
Copy link
Collaborator

@byara byara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @rsslldnphy and thank you for fixing this bug! Could you please add some snapshot tests for this case? After that, I'll merge this and we can release the fixes.

@rsslldnphy
Copy link
Contributor Author

Sure thing, have done! Thanks :-)

@rsslldnphy
Copy link
Contributor Author

hmm not sure why it's failing yarn --immutable with a 404 - works locally. maybe worth rerunning?

@byara byara merged commit 668adfa into trivago:v5.2.0 Dec 9, 2024
3 checks passed
@byara byara mentioned this pull request Dec 9, 2024
byara added a commit that referenced this pull request Dec 9, 2024
* Fix conditional import of prettier-plugin-svelte (#332)

* Fix type import ordering (#331)

* fix type import ordering

* Add a snapshot test for the bugfix

* clean up

---------

Co-authored-by: Russell Dunphy <russell@russelldunphy.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

import type { ... } from "./foo"; no longer gets sorted by importOrder
2 participants