GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Add refactor to convert namespace to named imports and back · Pull Request #24469 · microsoft/TypeScript

Conversation

Deleted user (ghost) commented May 29, 2018 •

edited by ghost

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Fixes #24044 and half of #19260

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

SlurpTheo reacted with thumbs up emoji

All reactions

👍 1 reaction


          Add refactor to convert namespace to named imports and back

fff5141

ghost requested a review from Andrew Casey (amcasey)

May 29, 2018 21:16

Mohamed Hegazy (mhegazy) reviewed

May 30, 2018

View reviewed changes

Comment thread

src/services/refactors/convertImport.ts

		const importDecl = getParentNodeInSpan(token, file, span);
		if (!importDecl \|\| !isImportDeclaration(importDecl)) return undefined;
		const { importClause } = importDecl;
		return importClause && importClause.namedBindings;

Mohamed Hegazy (mhegazy) May 30, 2018

Copy link

Copy Markdown

Contributor

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.

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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Deleted user (ghost) May 30, 2018

Copy link

Copy Markdown

Author

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.

Added a test. If just d is selected, getParentNodeInSpan will return just d and not the entire import declaration.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) reviewed

May 30, 2018

View reviewed changes

Comment thread

src/services/refactors/convertImport.ts Outdated


		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));

Mohamed Hegazy (mhegazy) May 30, 2018

Copy link

Copy Markdown

Contributor

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.

what if thsi is a type position? this needs to be a qualifiedName instead.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Deleted user (ghost) May 30, 2018

Copy link

Copy Markdown

Author

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.

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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) reviewed

May 30, 2018

View reviewed changes

Comment thread

src/services/utilities.ts Outdated

		function isFreeIdentifier(node: Identifier): boolean {
		const { parent } = node;
		switch (parent.kind) {
		case SyntaxKind.PropertyAccessExpression:

Mohamed Hegazy (mhegazy) May 30, 2018

Copy link

Copy Markdown

Contributor

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 am not sure i understand what this does.. and why not the name?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Deleted user (ghost) May 30, 2018

Copy link

Copy Markdown

Author

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.

Added a comment.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) reviewed

May 30, 2018

View reviewed changes

Comment thread

src/services/refactors/convertImport.ts Outdated


		const importedSymbols = toConvert.elements.map(e => checker.getSymbolAtLocation(e.name)!);

		forEachFreeIdentifier(sourceFile, id => {

Mohamed Hegazy (mhegazy) May 30, 2018

Copy link

Copy Markdown

Contributor

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.

why not just find all references on the ImportSpecifier and change these instead of looking at all identifiers..

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) May 30, 2018

Copy link

Copy Markdown

Contributor

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.

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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) reviewed

May 30, 2018

View reviewed changes

Comment thread

src/services/refactors/convertImport.ts

		forEachFreeIdentifier(sourceFile, id => {
		if (id === toConvert.name \|\| checker.getSymbolAtLocation(id) !== namespaceSymbol) return;

		if (!isPropertyAccessExpression(id.parent)) {

Mohamed Hegazy (mhegazy) May 30, 2018

Copy link

Copy Markdown

Contributor

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.

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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Add tests and comments

bcdec37

Mohamed Hegazy (mhegazy) commented May 30, 2018

Copy link

Copy Markdown

Contributor

thanks for the comment, but still why not findAllRefs?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Code review

61bef3e

Deleted user (ghost) commented

May 30, 2018

View reviewed changes

Comment thread

src/services/refactors/convertImport.ts Outdated


		function doChange(sourceFile: SourceFile, program: Program, changes: textChanges.ChangeTracker, toConvert: NamedImportBindings): void {
		const usedIdentifiers = createMap<true>();
		forEachFreeIdentifier(sourceFile, id => usedIdentifiers.set(id.text, true));

Deleted user (ghost) May 30, 2018

Copy link

Copy Markdown

Author

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.

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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) May 30, 2018

Copy link

Copy Markdown

Contributor

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.

D'oh.. did not think of this one.. i guess we need a walk after all.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) May 30, 2018

Copy link

Copy Markdown

Contributor

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.

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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andy Hanson added 2 commits

May 30, 2018 11:27


          Handle shorthand property assignment and re-export

4cc0474


          Don't use forEachFreeIdentifier

5bb8921

Mohamed Hegazy (mhegazy) approved these changes

May 30, 2018

View reviewed changes


          Fix rename after "."

bab662d

ghost force-pushed the refactorConvertImport branch from f40242d to bab662d Compare

May 30, 2018 20:27

ghost merged commit 43bf039 into master

May 30, 2018

ghost deleted the refactorConvertImport branch

May 30, 2018 21:11

ghost pushed a commit that referenced this pull request

Jun 4, 2018


          Port needed changes from #24469

6d41835

ghost pushed a commit that referenced this pull request

Jun 4, 2018


          moveToNewFile: Update namespace imports (#24612) (#24657)

4e17e77

* moveToNewFile: Update namespace imports (#24612)

* Port needed changes from #24469

Microsoft (microsoft) locked as resolved and limited conversation to collaborators

Oct 21, 2025

This pull request was closed.

Reviewers

Andrew Casey (amcasey)

Awaiting requested review from amcasey

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

1 participant

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

Deleted user (ghost) commented May 29, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mohamed Hegazy (mhegazy) commented May 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Deleted user (ghost) commented May 29, 2018 •

edited by ghost

Loading

Uh oh!

There was an error while loading. Please reload this page.