Fix organizeImports with type-only imports #36807
Conversation
|
@typescript-bot cherry-pick this to release-3.8 |
Component commits: ec495ab Fix organizeImports with type-only imports
|
Hey @DanielRosenwasser, I've opened #36810 for you. |
|
|
| const sortedImports = parseImports( | ||
| `import type { x } from "lib";`, | ||
| `import type * as y from "lib";`, | ||
| `import type z from "lib";`); |
There was a problem hiding this comment.
Why cant this mix with import type { x, default as z} from "lib just like normal import statement ?
There was a problem hiding this comment.
I believe we decided it would be ambiguous what the "type" keyword applied to and forced explicitness.
There was a problem hiding this comment.
It could, but I’m not confident that’s desirable. My personal preference for readability would probably be what’s represented here in the test.
There was a problem hiding this comment.
@andrewbranch Consider adding a comment about this: // Type-only imports are not allowed to combine (for readability). Maybe on the test too.
There was a problem hiding this comment.
Probably possible at a later pass, but seems reasonable to leave it the way it is now until we get further feedback.
There was a problem hiding this comment.
If this is a stylistic choice in organizeImports, I don't think "allowed" is the right word - I definitely read that as a language restriction.
There was a problem hiding this comment.
My comment in the implementation was actually referring to the language restriction, ignoring the possibility of rewriting the default as a named import. I’ll follow up to clarify.
Component commits: ec495ab Fix organizeImports with type-only imports Co-authored-by: Andrew Branch <[email protected]>
Fixes #36798
Not sure how I missed this originally 😱
There is room for opinions on how type-only imports should be sorted relative to non-type-only imports, but I’d suggest the time for those opinions is not 80 minutes before our release build 😄
Tip: review ignoring whitespace