Add refactor to convert namespace to named imports and back #24469
Conversation
| 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..
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.
|
|
||
| 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.
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.
src/services/utilities.ts
Outdated
| 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?
|
|
||
| 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..
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};
| 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.
|
thanks for the comment, but still why not findAllRefs? |
|
|
||
| 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.
There was a problem hiding this comment.
D'oh.. did not think of this one.. i guess we need a walk after all.
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.
ghost
force-pushed
the
refactorConvertImport
branch
from
f40242d
to
bab662d
Compare
Fixes #24044 and half of #19260