-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Move import sorting over to prettier #50618
Conversation
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.
lgtm!
b70ca08
to
d1f62e2
Compare
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.
The Prettier plugin makes more opinionated choices than the ESLint plugin which ultimately makes the configuration much simpler (it has mostly to do with how types are sorted).
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 added a commit with a bunch of proposed changes, let me know what you think.
- A clear split of our own packages between app packages and "library" packages. Taken from tsconfig.json to minimize the amount of places that need to be updated when something changes. gen-proto-ts is treated as a library package.
@gravitational/*
imports are grouped together with those without that prefix. - Node builtins go at the top. No strong opinion on this, it's just something we've been following in teleterm where we have the biggest amount of Node code. I guess it makes it easier to see that the code in the file is Node-only?
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.
On my machine, adding the sort plugin makes pnpm prettier-check
go from ~17-20s to ~27s. The ESLint plugin made pnpm eslint
slower by ~0-3s. I guess it's not necessarily an issue in the grand scheme of things, I just wanted us to be aware of this.
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 also see in the readme of this plugin that it "combines type and value imports (if importOrderTypeScriptVersion
is set to "4.5.0" or higher)". @kiosion I think you had some opinions on this?
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.
Ah, yeah. Not sure if it's still the case, would need to look into it again, but I remember there being some issues in the past regarding combined type/code imports and proper tree-shaking of unused imports... aside from that it's a stylistic preference (I find having all type-related imports grouped together is much more readable).
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.
FWIW, it should be possible to separate type imports from regular imports with this plugin, but we'd need to add a regex for each import group to avoid grouping all type imports. Grouping regular imports but not grouping type imports feels a bit off to me.
<TYPES>
- Not active by default, this allows you to group all type-imports, or target them with a regex (<TYPES>^[.]
targets imports of types from local files).
But the way the plugin does it right now is fine for me.
LGTM, let's resolve the conflicts and get it merged. Do we plan to backport this to avoid merge conflicts with backports? |
d8c9f55
to
50f8c68
Compare
Yeah |
@ryanclark See the table below for backport results.
|
This removes
eslint-plugin-import
and moves import sorting over to prettier instead.