-
Notifications
You must be signed in to change notification settings - Fork 25
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
Support explicit type sorting #44
Conversation
e16306a
to
3a54e59
Compare
@fbartho @blutorange I'd love to get your thoughts on this change before merging. |
If you would like to order type imports differently from value imports, you can use the special `<TYPES>` string. This example will place third party types at the top, followed by local types, then third party value imports, and lastly local value imports: | ||
|
||
```json | ||
"importOrder": ["<TYPES>", "<TYPES>^[./]", "<THIRD_PARTY_MODULES>", "^[./]"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love the "virtual matcher" combined with a custom matcher for Types. "<TYPES>^[./]"
It feels like a bad precedent to have to split these keys into tags and a regex, and have to match them by startsWith
, etc.
I don't mind the rest of this PR though! We talked about this feature previously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it does feel a bit wonky. Do you have any idea how else we could support it, though? (We don't actually use startsWith
here, fwiw, but we do remove the <TYPES>
part when creating the regex.
The tricky part here is giving the flexibility to declare a regex as only applying to type imports vs value imports. I think we'd need to support objects in importOrder
, and use something like importOrder: [{regex: '^[./]', isType: true}, ...]
, and that doesn't feel great either. I'm open to suggestions here, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so thinking about this more deeply, this is supposed to mirror the ability to create arbitrary groups for types. Eg. interleave types and non-type imports in batches.
I'd mayhaps try to argue that as a plugin for an opinionated formatter, maybe we should fight to remove the importOrder
option entirely, but that seems like a hard sell.
Fewer customization points = simpler code, and easier onboarding for users, so I guess your original proposal is better than the alternatives that jump to mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think that it's okay to keep importOrder
and try to minimize the number of other options that we expose. I definitely think we need a default for it that works in most cases, though.
Closes #35
Adapted from trivago/prettier-plugin-sort-imports#153, by @Xenfo.
This adds a new special string,
<TYPES>
that can be added to theimportOrder
array. When used, it will cause type imports to be sorted as specified by its location in the array.Notes:
importOrderCombineTypeAndValueImports
, throwing a warning if both are used. This is because:<TYPES>
is used, so that types can be sorted appropriately.importOrderCombineTypeAndValueImports
, and this change will give users a good way to opt-out of that behavior if they want, by specifying the location for<TYPES>
.