-
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
Feature: importOrderMergeDuplicateImports
#19
Conversation
Just use javascript or js
importOrderMergeDuplicateImports
- Fixes #4importOrderMergeDuplicateImports
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 have time for a full review right now, but thanks for hopping in and working on this. I did have one question that would be good to discuss, and I'll try to give a full review soon.
" | ||
`); | ||
}); | ||
it("doesn't merge duplicate imports if option disabled", () => { |
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.
Tests are always appreciated, it's great that you added some. But regarding snapshot tests, there's a special directory for those: https://github.com/IanVS/prettier-plugin-sort-imports/tree/main/tests that uses Jest snapshot testing. Could you move these tests there? That would keep it a bit more consistent.
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.
Snapshots tests are integration tests that check whether the plugin as a whole works as expected. https://github.com/IanVS/prettier-plugin-sort-imports/tree/main/src/utils/__tests__ is meant for unit tests of the single functions in the source code. And while having unit tests won't hurt, personally I agree with you and consider integration tests that check how the plugin works as a whole more important for this plugin.
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.
@blutorange I guess I'm confused as to the exact ask.
I'm definitely asserting specific functionality of the merge-nodes-with-matching-flavors, I just found it easier to validate the output by calling through the higher level API, because the output is just source-code, rather than somehow creating "artificial" arrays of nodes and shoving them into just the merge-function itself.
In this next PR: #20 I added a bunch of additional test cases to the same file -- those additional test cases came from here and those were indeed unit tests as well.
I think one cause of the organizational comments is that I'm using toMatchInlineSnapshot
for developer-updating convenience, rather than toEqual
as seen in the source-tests which requires the developer to update any changes by hand.
Options I see:
A. Rewrite tests to target lower level API mergeNodesWithMatchingImportFlavors
directly
B. Rewrite tests to use toEqual
as seen in other test cases and the source-tests
C. Rewrite tests to toMatchInlineSnapshot -> toMatchSnapshot
+ to move these tests to the special directory for traditional snapshots
What would you two like me to do?
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 think one cause of the organizational comments is that I'm using toMatchInlineSnapshot for developer-updating convenience, rather than toEqual as seen in the source-tests which requires the developer to update any changes by hand.
Personally I'm not a big fan of snapshot tests, normally. I think it works well for the integration tests, but I'd rather have explicit .toEqual()
in unit tests. They're a bit more of a pain to change, sure, but they're also harder to accidentally update just to make the tests pass without understanding the reason for the change.
So, I would vote for option B
above, for anything that's not in the top level tests
directory integration tests.
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, I'll convert all the toMatchInlineSnapshot
tests to a toEqual
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.
So basically, you are testing getCodeFromAst
and you only use prettier.format(...)
to clean up the code before the comparison (and you don't apply our plugin to the prettier.format(...)
call). Then yeah, I was confused by that and toMatchInlineSnapshot
.
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.
nodeToKeep.specifiers.push(...nodeToForget.specifiers); | ||
|
||
// The line numbers will be all messed up. Is this a problem? | ||
nodeToKeep.leadingComments = [ |
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'm wondering what exactly people would expect how comments are merged. Consider
// comment 1
// comment 2
import { foo } from "a"
// comment 3
// comment 4
// comment 5
// comment 6
import { bar } from "a"
// comment 7
// comment 8
which results in
// comment 1
// comment 2
// comment 3
// comment 4
// comment 5
// comment 6
import { foo, bar } from "a"
// comment 7
// comment 8
If one deletes the second import manually and adds the import to the first, one would get
// comment 1
// comment 2
import { foo, bar } from "a"
// comment 3
// comment 4
// comment 5
// comment 6
// comment 7
// comment 8
I can see an argument for both cases. Though I'd assume comments in the import section are rare in general and if they do occur, they are probably meant as an explanation for a particular import, so keeping comments on top sound like the right thing to do.
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 see both cases too. I think this is a fundamental tradeoff of using this feature of the plugin. Unless we want to disable merging imports if there are any leading/contained/trailing comments, I think this tradeoff is acceptable, but that's just one vote?
Looking at the few-hundred-file code-base that I'm applying this too, only a few comments got shifted, and only one of them was sensitive (@ts-expect-error
-- Singular exception allowing import of JS into TS even though our codebase doesn't normally allow that).
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 think that the behavior shown as current (all comments go above) is the "more correct" option. If that's what the code is doing now, I say we stick with it.
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 think the current behavior is reasonable, but maybe unintuitive. I think the root cause is that the parser prefers to assign comments to the leading
position than the trailing
position?
So comments 3, 4, 5, 6 are leading
comments for the 2nd import, and my code moves them to be leading
comments for the 1st import.
If comments 3-6 were considered trailing
comments for the 1st import, then the behavior would have matched the manual
process and maybe been a hair more intuitive?
Again, I agree the way it behaves now is probably the "more correct" option even if it doesn't match how a human would visualize the operations. Leading vs trailing comments is a weird thing to specify tbh.
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.
/shrug it's intuitive to me, if we're combining the comments above each of the imports to remain above the combined import. I also think this is a super rare edge case, and probably not worth spending too much effort on perfecting. ;)
|
||
nodeToKeep.specifiers.push(...nodeToForget.specifiers); | ||
|
||
// The line numbers will be all messed up. Is this a problem? |
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.
There's the part where it removes nodes from the original code which relies on the line number being correct (it did so even before the linked commit).
It is called from
const codeWithoutImportsAndInterpreter = removeNodesFromOriginalCode( |
I think that might be alright since it should operate on the original nodes, but perhaps you could check again that the nodes passed to that function are not the modified ones?
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 nodes are both the modified & the original ones -- exactly line: cf4e6af#diff-ab5d98b73e57fb0fe6cb1b2b9ea7d14530182d5d1987eae7c545bd15f4528cfeR37
The trick is that the modified nodes don't have their line-numbers adjusted. The line-numbers still match what's in the provided code.
So… I think we're good?
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.
Yes, I think so. If not, and if it did remove code at the wrong position, then the tests would have failed already
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 quite understand this, and it relates to my comment from earlier about why we're providing both the modified and original nodes. But, feel free to ignore me, if y'all think it's fine how it is.
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.
That comment was a bit out of date, as I've now learned that that's actually an important attribute of how getCodeFromAst
functions. It needs the old line-numbers/loc to properly move things around!
// These mutations don't update the line numbers, and that's crucial for moving things around.
// To get updated line-numbers you would need to re-parse the code after these changes are rendered!
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.
Overall I think this looks good. I had a few minor comments and questions, but I think this is pretty close to mergable. Thanks for the work on this, @fbartho.
src/preprocessor.ts
Outdated
if (importNodes.length === 0) { | ||
return code; | ||
} | ||
|
||
const allImports = getSortedNodes(importNodes, { | ||
const remainingImports = getSortedNodes(importNodes, { |
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.
Question, why the change to the name here? Remaining after what? Is this because getSortedNodes
will now also combine/remove some nodes?
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.
Yes, the return value of getSortedNodes
is no longer "all imports" it's just the "imports that weren't deleted"
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'll rename this inline with renames I'm doing to match your feedback on naming of parameters to getCodeFromAst
src/utils/get-code-from-ast.ts
Outdated
) => { | ||
export const getCodeFromAst = ({ | ||
nodes, | ||
importNodes = nodes, |
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 naming here feels a little bit odd. It's not immediately clear what the difference should be between nodes
and importNodes
, since nodes
is also a list of import nodes.
is importNodes
a superset of nodes
? Why do we need to pass both in here, since both are being spread into an array? Or could we just pass importNodes
, if it contains all of nodes
, plus additional ones?
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.
Yes, importNodes
is a superset, we need to pass the original nodes so that the old code-locations can be deleted when we merge a node. Once I was deleting some of the nodes from the nodes
-array then the generated output code had the properly sorted & modified nodes followed by the unmodified nodes I wanted to have deleted.
How do you feel about these names?
/**
* This function generates a code string from the passed nodes.
* @param nodesToOutput The remaining imports which should be rendered. (Node specifiers & types may be mutated)
* @param allOriginalImportNodes All import nodes that were originally relevant. (This includes nodes that need to be deleted!)
* @param originalCode The original input code that was passed to this plugin.
* @param directives All directive prologues from the original code (e.g.
* `"use strict";`).
* @param interpreter Optional interpreter directives, if present (e.g.
* `#!/bin/node`).
*/
export const getCodeFromAst = ({
nodesToOutput,
allOriginalImportNodes = nodesToOutput,
originalCode,
directives,
interpreter,
}: {
nodesToOutput: Statement[];
allOriginalImportNodes?: Statement[];
originalCode: string;
directives: Directive[];
interpreter?: InterpreterDirective | null;
}) => {
import { default as Def2 } from 'd'; | ||
import { default as Def1 } from 'd'; | ||
import Foo1 from 'e'; | ||
import Foo2 from 'e'; |
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.
There's a ton going on here, it might make the test more readable if it were broken into separate test cases.
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.
More granular unit tests were submitted in #20
}, | ||
{ | ||
[importFlavorRegular]: [] as ImportDeclaration[], | ||
[importFlavorType]: [] as ImportDeclaration[], |
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.
Typecasting works and is probably fine here, but normally I prefer to use a generic on the reduce
call, like
return nodes.reduce<{[key: typeof importFlavorRegular | typeof importFlavorType]: ImportDeclaration[]}>(
That's a bit clunky in this case, but it lets us avoid using as
. Go ahead and leave this as-is, though, just wanted to mention it.
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.
Did it, but slightly differently!
|
||
nodeToKeep.specifiers.push(...nodeToForget.specifiers); | ||
|
||
// The line numbers will be all messed up. Is this a problem? |
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 quite understand this, and it relates to my comment from earlier about why we're providing both the modified and original nodes. But, feel free to ignore me, if y'all think it's fine how it is.
Try to improve the naming and docs for the parameters, particularly the nodes vs importNodes distinciton
I will give this a final review soon. I have been on a work event for a few days, and have some other things to catch up on, but will try to get to it this week. |
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.
Great work here, I pushed a small commit to add a couple of tests to convince myself everything's working correctly, and a few small comments to remind myself what is going on when I look at this again in the future.
This PR implements import declaration merging for duplicate imports.
getCodeFromAST
so that we could still provide the original nodes, even if some of the nodes were being deleted.side-effect
import, or aprettier-ignore
import, it won't merge across that boundary.import type { stuff }
intoimport { type stuff }
as it was not obvious when this would be wise. If we want to control that with an additional config option, something likeimportOrderMergeTypeStyle: "none" | "to-normal-import" | "unmerge"
I could be convinced. I forsee people wanting opposite sides of the coin for that one.Please let me know what you think and if there's any edge-cases I missed!