Add refactor to convert namespace to named imports and back - #24469
Conversation
Sorry, something went wrong.
| const importDecl = getParentNodeInSpan(token, file, span); | ||
| if (!importDecl || !isImportDeclaration(importDecl)) return undefined; | ||
| const { importClause } = importDecl; | ||
| return importClause && importClause.namedBindings; |
There was a problem hiding this comment.
should we verify that the namedBindings is within the span..
e.g. selecting d in import d, * as ns from "./mod" should not trigger any action..
Sorry, something went wrong.
There was a problem hiding this comment.
Added a test. If just d is selected, getParentNodeInSpan will return just d and not the entire import declaration.
Sorry, something went wrong.
|
|
||
| forEachFreeIdentifier(sourceFile, id => { | ||
| if (!toConvert.elements.some(e => e.name === id) && contains(importedSymbols, checker.getSymbolAtLocation(id))) { | ||
| changes.replaceNode(sourceFile, id, createPropertyAccess(createIdentifier(namespaceImportName), nameToPropertyName.get(id.text) || id.text)); |
There was a problem hiding this comment.
what if thsi is a type position? this needs to be a qualifiedName instead.
Sorry, something went wrong.
There was a problem hiding this comment.
Added a test -- this should work as well either way since we're just changing text and not creating a new tree.
Sorry, something went wrong.
| function isFreeIdentifier(node: Identifier): boolean { | ||
| const { parent } = node; | ||
| switch (parent.kind) { | ||
| case SyntaxKind.PropertyAccessExpression: |
There was a problem hiding this comment.
i am not sure i understand what this does.. and why not the name?
Sorry, something went wrong.
There was a problem hiding this comment.
Added a comment.
Sorry, something went wrong.
|
|
||
| const importedSymbols = toConvert.elements.map(e => checker.getSymbolAtLocation(e.name)!); | ||
|
|
||
| forEachFreeIdentifier(sourceFile, id => { |
There was a problem hiding this comment.
why not just find all references on the ImportSpecifier and change these instead of looking at all identifiers..
Sorry, something went wrong.
There was a problem hiding this comment.
also we need to handle cases where the propertyAccess expression will not work.. e.g. shorthandproperties, var x = {a}, changing it to var x = { m.a } will not work, you need to change it to var x = { a: m.a }. similarly the export declarations need to be handled separately.. so export {a} will have to be rewritten either as export {a} from "mod" or import a = m.a; export {a};
Sorry, something went wrong.
| forEachFreeIdentifier(sourceFile, id => { | ||
| if (id === toConvert.name || checker.getSymbolAtLocation(id) !== namespaceSymbol) return; | ||
|
|
||
| if (!isPropertyAccessExpression(id.parent)) { |
There was a problem hiding this comment.
what if it is used in a call expression.. import * as n from "mod"; n();?
if allowSyntheticDefaultImport or esModuleInterop are on, then you can change it to import {default as n} from "mod"; n();, if not, we need to keep the namespace import around.. do not think there is another way to handle that.
Sorry, something went wrong.
|
thanks for the comment, but still why not findAllRefs? |
Sorry, something went wrong.
|
|
||
| function doChange(sourceFile: SourceFile, program: Program, changes: textChanges.ChangeTracker, toConvert: NamedImportBindings): void { | ||
| const usedIdentifiers = createMap<true>(); | ||
| forEachFreeIdentifier(sourceFile, id => usedIdentifiers.set(id.text, true)); |
There was a problem hiding this comment.
Looks like isFileLevelUniqueName isn't suitable for this purpose since sourceFile.identifiers includes the text of every Identifier node in the source file, including the names in property accesses. So if we access import * as m from "m"; m.x then that would make us convert it to import { x as _ x } from "m"; unnecessarily.
Sorry, something went wrong.
There was a problem hiding this comment.
D'oh.. did not think of this one.. i guess we need a walk after all.
Sorry, something went wrong.
There was a problem hiding this comment.
one other option is to call resolveName for the property name on every reference for the namespace import, if it resolves to anything then you know you need to alias it.
Sorry, something went wrong.
Fixes #24044 and half of #19260