Skip to content

For import fix, support path mapping value that begins in "./" or ends in ".ts"#21035

Merged
6 commits merged into
masterfrom
importnameCodeFixNewImportPaths_withExtension
Jan 9, 2018
Merged

For import fix, support path mapping value that begins in "./" or ends in ".ts"#21035
6 commits merged into
masterfrom
importnameCodeFixNewImportPaths_withExtension

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Jan 5, 2018

Fixes #20525

@ghost ghost requested a review from amcasey January 5, 2018 21:05
Copy link
Copy Markdown
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know that it matters, but it appears that this won't work for paths starting with ././.

Comment thread src/services/codefixes/importFixes.ts Outdated

/** Convert "./x" (and "././x") to "x". */
function removeLeadingDotSlash(path: string): string {
return startsWith(path, "./") || startsWith(path, ".\\") ? removeLeadingDotSlash(path.slice(2)) : path;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the two relative paths are not relative to the same thing.. the input is relative to the current file, but the path is relative to the tsconfig.json. do not think removing the ./ is the right thing to do here. you probably want to expand both, or normalize one in terms of the other if it starts with a ./ or a ../

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two parameters to tryGetModuleNameFromPaths are relative to the baseUrl. I pushed a commit that updates their names.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh.. good to know :)

what about ..\?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, that needed a fix too.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can u use getNormalizedPathComponents instead?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this function is unused! #21092

@ghost ghost force-pushed the importnameCodeFixNewImportPaths_withExtension branch from 6908254 to 0ca56de Compare January 8, 2018 22:31
@ghost ghost merged commit 82fb294 into master Jan 9, 2018
@ghost ghost deleted the importnameCodeFixNewImportPaths_withExtension branch January 9, 2018 18:34
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants