-
Notifications
You must be signed in to change notification settings - Fork 21
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
Feature: importOrderCombineTypeAndValueImports
#20
Conversation
Just use javascript or js
Fixes: #4 Allows the ability for users to convert `import type` expressions into `import {type …}` expressions via a boolean flag. Should this be controlled via a string parameter?
I think we need to talk about whether this warrants yet another option, but in the meantime, do you think you could add some tests? You might be able to use the test cases from my codemod as a starting point: https://github.com/IanVS/type-import-codemod/blob/9f3210861a77216f07317537a04ce61467e0df7e/src/transform.test.ts |
Happy to add those tests. I do tend to have macro-tests that assert a ton of functionality all at once. Not sure that's a good sign, but code-coverage likes it 😉 |
@IanVS Test cases imported, the only differences I can see
Do we need/want to group type imports when sorting? |
Try to improve the naming and docs for the parameters, particularly the nodes vs importNodes distinciton
…ts-with-regular-imports
Sorry, it looks like there are a number of conflicts on this PR now. Your best bet might be to reset hard to main, and then cherry-pick the commits from this PR back onto the branch. |
# Conflicts: # README.md # src/index.ts # src/preprocessor.ts # src/types.ts # src/utils/__tests__/get-all-comments-from-nodes.spec.ts # src/utils/__tests__/get-code-from-ast.spec.ts # src/utils/__tests__/get-sorted-nodes-by-import-order.spec.ts # src/utils/__tests__/get-sorted-nodes.spec.ts # src/utils/__tests__/merge-nodes-with-matching-flavors.spec.ts # src/utils/__tests__/remove-nodes-from-original-code.spec.ts # src/utils/get-sorted-nodes.ts # src/utils/merge-nodes-with-matching-flavors.ts
@IanVS sorry about the lag re: updates, I was under the weather last week! Please take another look, and let me know if this can proceed. |
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.
It took me a while to get to this, sorry about that. I think it mostly looks good. I had a few small comments/questions.
Unless you or @blutorange feels strongly, I'd like to sort the type imports to the end. I think it looks better that way, and is more readable. Would you mind making that change? Then we can merge and release this, and prepare for the breaking changes release.
src/preprocessor.ts
Outdated
!importOrderMergeDuplicateImports | ||
) { | ||
console.warn( | ||
"[@ianvs/prettier-plugin-sort-imports]: Option combination of both importOrderMergeTypeImportsIntoRegular: true and importOrderMergeDuplicateImports: false is not won't do anything!", |
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.
"[@ianvs/prettier-plugin-sort-imports]: Option combination of both importOrderMergeTypeImportsIntoRegular: true and importOrderMergeDuplicateImports: false is not won't do anything!", | |
"[@ianvs/prettier-plugin-sort-imports]: Enabling importOrderMergeTypeImportsIntoRegular will have no effect unless importOrderMergeDuplicateImports is also enabled.", |
function nodeIsImportNamespaceSpecifier( | ||
node: ImportSpecifier | ImportDefaultSpecifier | ImportNamespaceSpecifier, | ||
): node is ImportNamespaceSpecifier { | ||
return node.type === 'ImportNamespaceSpecifier'; | ||
} | ||
|
||
/** import Default from "someModule" */ |
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.
Why remove the comments?
return node.type === 'ImportSpecifier'; | ||
} | ||
|
||
function convertImportSpecifierType(node: ImportSpecifier) { |
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.
Can we call this convertImportSpecifierToType
?
Hi @fbartho. How would you like to proceed on this one? We can merge it as-is if you don't have time to make changes, and I can play around with re-ordering the type/value import specifiers in a separate PR. |
Hiya! I was traveling 2 weeks ago, and totally missed these messages! No objections to the changes, I’m happy to make them — should be able to get to them this week or this coming Saturday |
Great, thanks! I hope you had a good time on your travels! |
Hi @fbartho just checking in to see if this is still something you'll be able to get to. NBD if not, just was wondering. |
@IanVS whoops! It's been a little bit of a scramble. I'll take a look at this, this week! |
Thanks for your work on this @fbartho! I pushed up a few tweaks, and am going to merge / release. We can continue to iterate, but I'd like to start preparing for a breaking release, to include prettier 3 support. |
importOrderMergeTypeImportsIntoRegular
importOrderCombineTypeAndValueImports
Fixes: #4
Allows the ability for users to convert
import type
expressions intoimport {type …}
expressions via a boolean flag.Should this be controlled via a string parameter?