Canonicalize path before calling `startsWith` · Pull Request #25364 · microsoft/TypeScript

Deleted user (ghost) · 2018-07-02T15:14:41Z

Fixes microsoft/vscode#52089 and #25279

Daniel Rosenwasser (DanielRosenwasser) · 2018-07-02T16:52:55Z

What about test cases? Also, does this fix any of #25358 #24599 #25345 #25186?

Wesley Wigham (weswigham) · 2018-07-02T17:00:10Z

I do agree with Daniel that we probably should err on adding more test coverage tho, considering this is used for paths in declaration emit now, too.

Sheetal Nandi (sheetalkamat) · 2018-07-02T18:56:53Z

        // Get a path that's relative to node_modules or the importing file's path
        // if node_modules folder is in this folder or any of its parent folders, no need to keep it.
-        if (!startsWith(sourceDirectory, moduleSpecifier.substring(0, parts.topLevelNodeModulesIndex))) return undefined;
+        if (!startsWith(sourceDirectory, getCanonicalFileName(moduleSpecifier.substring(0, parts.topLevelNodeModulesIndex)))) return undefined;


Dont you need to do getCanonicalFileName for sourceDirectory as well?

That comes from importingSourceFile.path which should already be canonicalized.

Makes sense. Good that you changed the type there so its easy to catch in future. Thanks.

Mohamed Hegazy (mhegazy) · 2018-07-02T19:41:22Z

Also looks related to #25338

Deleted user (ghost) · 2018-07-03T16:54:13Z

Sheetal Nandi (@sheetalkamat) Do we have any existing case-insensitivity tests? I thought fourslash was case-insensitive but I can't reproduce the error in a fourslash test.

Sheetal Nandi (sheetalkamat) · 2018-07-03T17:27:09Z

you can add it in tsserver unit tests where default host is case insensitive

Deleted user (ghost) · 2018-07-03T17:29:23Z

Figured out why fourslash test wasn't working, I needed to put a capitalized name before the importing directory.

Mohamed Hegazy (mhegazy) · 2018-07-03T19:28:44Z

Andy (Andrewkraft) (@Andy-MS) can you also verify that this change fixes #25358 #24599 #25345 and #25186?

Wenlu Wang (Kingwl) · 2018-07-05T07:40:29Z

seems fixed on my pc@20180705

Marco Polichetti (gremo) · 2018-07-09T07:27:26Z

I'm running vscode 1.25.0 and typscript 2.9.2, still have this issue. Can anyone help? Thanks

Wenlu Wang (Kingwl) · 2018-07-10T08:11:34Z

I'm running vscode 1.25.0 and typscript 2.9.2, still have this issue. Can anyone help? Thanks

install the typescript@next
set typescript tsdk to gloabl typescript lib, eg: /usr/local/lib/node_modules/typescript/lib

Simon Bobrov (smnbbrv) · 2018-07-10T10:59:42Z

This is all cool, but angular cli does not allow to use the newest typescript. What should we do?

Deleted user (ghost) · 2018-07-10T22:13:18Z

Simon Bobrov (@smnbbrv) Just install typescript globally with npm install -g typescript@next and point the editor to that.

Simon Bobrov (smnbbrv) · 2018-07-11T14:17:53Z

Andy (Andrewkraft) (@Andy-MS) thanks, it works!

Rati Dzidziguri (ratidzidziguri) · 2018-07-11T20:17:27Z

for me installing @next did not work

and still when importing

Mohamed Hegazy (mhegazy) · 2018-07-11T20:28:36Z

Rati Dzidziguri (@ratidzidziguri) can you share more info.

Rati Dzidziguri (ratidzidziguri) · 2018-07-11T21:34:50Z

solution for now was to only set VsCode to use workplace TS version. In my case that is the one installed with angular and it is 2.7.2

Simon Bobrov (smnbbrv) · 2018-07-12T18:17:09Z

Maybe I say bullshit but if the typescript version is so crucial why don’t you guys just bundle it with vscode or at least give users some hint about mismatching typescript versions and the consequences of that? The way how it is done right now is so counter intuitive; it was properly working one version before, then it gets broken and the only way to fix this is using beta versions of typescript; that’s kinda... weird

Mohamed Hegazy (mhegazy) · 2018-07-12T18:20:57Z

The next version of VSCode should include the fix. VSCode insiders tomorrow should include the fix.

If you are on latest VSCode and want to try the fix out, you can find instructions on using a custom version of TS in your VSCode at: Using Newer TypeScript Versions.

Peter Weinberg (no-stack-dub-sack) · 2018-07-14T23:31:55Z

Andy (Andrewkraft) (@Andy-MS) I just installed the latest insiders and am on TS 2.9.2 - you mentioned the new insiders should have the fix - should 2.9.2 + latest insiders be working? I'm still getting the same behavior as when I originally opened the issue.

Peter Weinberg (no-stack-dub-sack) · 2018-07-14T23:44:23Z

☝️ never mind, it does work when I update to @next. So for those dealing with this issue that aren't using insiders and tsnext, will they have to wait until 3.0.0 is shipping with VSCode?

Rati Dzidziguri (ratidzidziguri) · 2018-07-15T15:25:06Z

For angular developers. till we get some fix, simple solution is to use work-space version instead of VsCodes one which is 2.7.2 in my case and everything works normally after that.

Lucas Klaassen (lucasklaassen) · 2018-07-19T17:37:29Z

Rati Dzidziguri (@ratidzidziguri) can you explain what you mean by using a "work-space" version?

alamothe · 2018-07-28T01:47:03Z

This last release of VS Code is so, so buggy.

Hope an upgrade is coming soon!

Does it make sense to allow people to revert after so many bugs were introduced?

Wesley Wigham (weswigham) · 2018-07-28T02:12:35Z

You can always set your workspace version of TS to an older (or newer!) version of TS.

hobojoe · 2018-07-30T18:10:09Z

Rati Dzidziguri (@ratidzidziguri) can you explain what you mean by using a "work-space" version?

Lucas Klaassen (@lucasklaassen) You can click on the right bottom toolbar of VS Code. Where you have the typescript version. From there you can choose the workspace version.

Dave Jensen (djensen47) · 2018-08-08T20:12:35Z

Is there any chance this can get into a patch release? This apparently fixes #52089? Which is a pretty bad regression, it's loss of functionality. Basically auto imports import the wrong file.

Thanks for all the hard work on this project. VS Code is awesome.

Canonicalize path before calling startsWith

4363ca4

ghost requested a review from Sheetal Nandi (sheetalkamat) July 2, 2018 15:14

Wesley Wigham (weswigham) approved these changes Jul 2, 2018

View reviewed changes

Mohamed Hegazy (mhegazy) approved these changes Jul 2, 2018

View reviewed changes

Sheetal Nandi (sheetalkamat) reviewed Jul 2, 2018

View reviewed changes

ghost force-pushed the pathStartsWith branch from e36f1b9 to 0294132 Compare July 3, 2018 17:25

More specific type for sourceDirectory, and add fourslash test

140ebcd

Sheetal Nandi (sheetalkamat) approved these changes Jul 3, 2018

View reviewed changes

ghost force-pushed the pathStartsWith branch from 0294132 to 140ebcd Compare July 3, 2018 17:28

Update API (#24966)

db03a99

ghost merged commit 304d45d into master Jul 3, 2018

ghost deleted the pathStartsWith branch July 3, 2018 18:23

This was referenced Jul 3, 2018

automatic import confuses relative paths and package imports #25279

Closed

import suggestion for node modules includes entire path microsoft/vscode#52089

Closed

This was referenced Jul 3, 2018

Typescript style-only imports are not resolving correctly #25358

Closed

Import types in d.ts files contain relative paths to node_modules #24599

Closed

typescript@next (and 2.9.x) outputs node_modules relative path to imports #25345

Closed

Wesley Wigham (weswigham) mentioned this pull request Jul 20, 2018

In certain cases TS 2.9 outputs incorrect definitions (.d.ts) #25511

Closed

Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 21, 2025

This pull request was closed.

Conversation

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

Uh oh!

Uh oh!

Daniel Rosenwasser (DanielRosenwasser) commented Jul 2, 2018

Uh oh!

Wesley Wigham (weswigham) commented Jul 2, 2018

Uh oh!

Sheetal Nandi (sheetalkamat) Jul 2, 2018

Choose a reason for hiding this comment

Uh oh!

Deleted user (ghost) Jul 3, 2018

Choose a reason for hiding this comment

Uh oh!

Sheetal Nandi (sheetalkamat) Jul 3, 2018

Choose a reason for hiding this comment

Uh oh!

Mohamed Hegazy (mhegazy) commented Jul 2, 2018

Uh oh!

Deleted user (ghost) commented Jul 3, 2018

Uh oh!

Sheetal Nandi (sheetalkamat) commented Jul 3, 2018

Uh oh!

Deleted user (ghost) commented Jul 3, 2018

Uh oh!

Mohamed Hegazy (mhegazy) commented Jul 3, 2018

Uh oh!

Wenlu Wang (Kingwl) commented Jul 5, 2018

Uh oh!

Marco Polichetti (gremo) commented Jul 9, 2018

Uh oh!

Wenlu Wang (Kingwl) commented Jul 10, 2018

Uh oh!

Simon Bobrov (smnbbrv) commented Jul 10, 2018

Uh oh!

Deleted user (ghost) commented Jul 10, 2018

Uh oh!

Simon Bobrov (smnbbrv) commented Jul 11, 2018

Uh oh!

Rati Dzidziguri (ratidzidziguri) commented Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mohamed Hegazy (mhegazy) commented Jul 11, 2018

Uh oh!

Rati Dzidziguri (ratidzidziguri) commented Jul 11, 2018

Uh oh!

Simon Bobrov (smnbbrv) commented Jul 12, 2018

Uh oh!

Mohamed Hegazy (mhegazy) commented Jul 12, 2018

Uh oh!

Peter Weinberg (no-stack-dub-sack) commented Jul 14, 2018

Uh oh!

Peter Weinberg (no-stack-dub-sack) commented Jul 14, 2018

Uh oh!

Rati Dzidziguri (ratidzidziguri) commented Jul 15, 2018

Uh oh!

Lucas Klaassen (lucasklaassen) commented Jul 19, 2018

Uh oh!

alamothe commented Jul 28, 2018

Uh oh!

Wesley Wigham (weswigham) commented Jul 28, 2018

Uh oh!

hobojoe commented Jul 30, 2018

Uh oh!

Dave Jensen (djensen47) commented Aug 8, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

Deleted user (ghost) commented Jul 2, 2018 •

edited by ghost

Loading

Rati Dzidziguri (ratidzidziguri) commented Jul 11, 2018 •

edited

Loading