Ensure that the comma is removed when all named imports are removed via moveToFile #32663
Conversation
| @@ -351,6 +351,8 @@ namespace ts.refactor { | |||
| if (namedBindings) { | |||
| if (namedBindingsUnused) { | |||
| changes.delete(sourceFile, namedBindings); | |||
| // Also include the ", ": | |||
| changes.replaceRangeWithText(sourceFile, { pos: namedBindings.pos - 1, end: namedBindings.pos }, ""); | |||
There was a problem hiding this comment.
I would probably opt for
changes.replaceNode(
sourceFile,
importDecl.importClause,
updateImportClause(importDecl.importClause, name, /*namedBindings*/ undefined));(to replace both lines in this if)
There was a problem hiding this comment.
I think the issue is that the comma token is a child of the ImportClause, and TextChanges only does the exact thing you tell it to do without being smart about the context it’s working in. As you discovered, deleting the named bindings node just removes the text from pos to end, which doesn’t include the comma. So really, it's the ImportClause that needs to change—it needs to drop its named bindings node and its comma token. Replacing it with a new one will re-print its contents, which lets the emitter be smart about whether a comma is needed without requiring manual intervention.
There was a problem hiding this comment.
Perfect, yep - moving up the AST makes a lot of sense.
That change makes a lot of sense. Thanks 👯♂
…ia moveToFile - fixes microsoft#31195
Fixes #31195
This one feels like I'm taking a shortcut, but it was definitely doing the right thing before. When I looked in ast-explorer what
import defaultD, from "./has-exports";looks, like the comma comes from aNamedImportswith no elements inside aImportClause- removing the comma completely removes theNamedImportsnode.This AST change is actually happening in the current refactor as-is, however, deleting just that node doesn't include the comma (maybe the fix is instead to make the span for that node different?) but this will remove the comma either way. I don't believe there are ever cases where you need that comma.