Organize imports: don’t delete import declarations used for module augmentation#31482
Conversation
…ve their import clauses
|
|
||
| function hasModuleDeclarationMatchingSpecifier(sourceFile: SourceFile, moduleSpecifier: Expression) { | ||
| const moduleSpecifierText = isStringLiteral(moduleSpecifier) && moduleSpecifier.text; | ||
| return isString(moduleSpecifierText) && some(sourceFile.statements, statement => |
There was a problem hiding this comment.
Do you need to handle the new patternAmbientModule as well apart from the m,odule specifier text
There was a problem hiding this comment.
Hmm, I don't think so, but I'm not sure exactly what rule says why. Can you come up with an example where that would be necessary? E.g., this doesn't work:
import 'typescript';
declare module 'type*' {
interface Node {
mySpecialNodeAugmentation: string;
}
}| } | ||
| // ==ORGANIZED== | ||
|
|
||
| import 'caseless'; |
There was a problem hiding this comment.
Changing this to a side-effect import affects the emitted JS. Previously the import was elided due to being unused. With this slightly different syntax it will always be emitted.
There was a problem hiding this comment.
Ah, interesting. We can do this in declaration files (which should be the norm), but either do import {} from 'caseless' or just don’t touch it in TypeScript files?
There was a problem hiding this comment.
Addressed by not messing with imports for module augmentation outside of declaration files
…iles since it could change JS emit
| importDecl.modifiers, | ||
| /*importClause*/ undefined, | ||
| moduleSpecifier)); | ||
| } |
There was a problem hiding this comment.
Does this need to be instead updateImportDeclarationAndClause
There was a problem hiding this comment.
No, that adds an empty import clause instead of leaving it off.
amcasey
left a comment
There was a problem hiding this comment.
LGTM, modulo Sheetal's comments.
Fixes #31338
Import clauses are removed, e.g.
now becomes
(Previously, the whole import declaration was deleted.)