Canonicalize path before calling `startsWith` #25364

ghost · 2018-07-02T15:14:41Z

Fixes microsoft/vscode#52089 and #25279

DanielRosenwasser · 2018-07-02T16:52:55Z

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

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.

sheetalkamat · 2018-07-02T18:56:53Z

src/compiler/moduleSpecifiers.ts

@@ -290,7 +290,7 @@ namespace ts.moduleSpecifiers {
        const moduleSpecifier = getDirectoryOrExtensionlessFileName(moduleFileName);
        // 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.

mhegazy · 2018-07-02T19:41:22Z

Also looks related to #25338

ghost · 2018-07-03T16:54:13Z

@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.

sheetalkamat · 2018-07-03T17:27:09Z

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

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.

mhegazy · 2018-07-03T19:28:44Z

@andy-ms can you also verify that this change fixes #25358 #24599 #25345 and #25186?

Kingwl · 2018-07-05T07:40:29Z

seems fixed on my pc@20180705

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

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

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?

ghost · 2018-07-10T22:13:18Z

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

smnbbrv · 2018-07-11T14:17:53Z

@andy-ms thanks, it works!

ratidzidziguri · 2018-07-11T20:17:27Z

for me installing @next did not work

and still when importing

mhegazy · 2018-07-11T20:28:36Z

@ratidzidziguri can you share more info.

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

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

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.

no-stack-dub-sack · 2018-07-14T23:31:55Z

@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.

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?

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.

lucasklaassen · 2018-07-19T17:37:29Z

@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?

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

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

@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.

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 sheetalkamat July 2, 2018 15:14

weswigham approved these changes Jul 2, 2018

View reviewed changes

mhegazy approved these changes Jul 2, 2018

View reviewed changes

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

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

weswigham mentioned this pull request Jul 20, 2018

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

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Canonicalize path before calling `startsWith` #25364

Canonicalize path before calling `startsWith` #25364

ghost commented Jul 2, 2018 •

edited by ghost

Loading

DanielRosenwasser commented Jul 2, 2018

weswigham commented Jul 2, 2018

sheetalkamat Jul 2, 2018

ghost Jul 3, 2018

sheetalkamat Jul 3, 2018

mhegazy commented Jul 2, 2018

ghost commented Jul 3, 2018

sheetalkamat commented Jul 3, 2018

ghost commented Jul 3, 2018

mhegazy commented Jul 3, 2018

Kingwl commented Jul 5, 2018

gremo commented Jul 9, 2018

Kingwl commented Jul 10, 2018

smnbbrv commented Jul 10, 2018

ghost commented Jul 10, 2018

smnbbrv commented Jul 11, 2018

ratidzidziguri commented Jul 11, 2018 •

edited

Loading

mhegazy commented Jul 11, 2018

ratidzidziguri commented Jul 11, 2018

smnbbrv commented Jul 12, 2018

mhegazy commented Jul 12, 2018

no-stack-dub-sack commented Jul 14, 2018

no-stack-dub-sack commented Jul 14, 2018

ratidzidziguri commented Jul 15, 2018

lucasklaassen commented Jul 19, 2018

alamothe commented Jul 28, 2018

weswigham commented Jul 28, 2018

hobojoe commented Jul 30, 2018

djensen47 commented Aug 8, 2018

Canonicalize path before calling startsWith #25364

Canonicalize path before calling startsWith #25364

Conversation

ghost commented Jul 2, 2018 • edited by ghost Loading

DanielRosenwasser commented Jul 2, 2018

weswigham commented Jul 2, 2018

sheetalkamat Jul 2, 2018

Choose a reason for hiding this comment

ghost Jul 3, 2018

Choose a reason for hiding this comment

sheetalkamat Jul 3, 2018

Choose a reason for hiding this comment

mhegazy commented Jul 2, 2018

ghost commented Jul 3, 2018

sheetalkamat commented Jul 3, 2018

ghost commented Jul 3, 2018

mhegazy commented Jul 3, 2018

Kingwl commented Jul 5, 2018

gremo commented Jul 9, 2018

Kingwl commented Jul 10, 2018

smnbbrv commented Jul 10, 2018

ghost commented Jul 10, 2018

smnbbrv commented Jul 11, 2018

ratidzidziguri commented Jul 11, 2018 • edited Loading

mhegazy commented Jul 11, 2018

ratidzidziguri commented Jul 11, 2018

smnbbrv commented Jul 12, 2018

mhegazy commented Jul 12, 2018

no-stack-dub-sack commented Jul 14, 2018

no-stack-dub-sack commented Jul 14, 2018

ratidzidziguri commented Jul 15, 2018

lucasklaassen commented Jul 19, 2018

alamothe commented Jul 28, 2018

weswigham commented Jul 28, 2018

hobojoe commented Jul 30, 2018

djensen47 commented Aug 8, 2018

Canonicalize path before calling `startsWith` #25364

Canonicalize path before calling `startsWith` #25364

ghost commented Jul 2, 2018 •

edited by ghost

Loading

ratidzidziguri commented Jul 11, 2018 •

edited

Loading