Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unexpected changes to in-memory file paths parsing #134304

Closed
merceyz opened this issue Oct 1, 2021 · 7 comments
Closed

Unexpected changes to in-memory file paths parsing #134304

merceyz opened this issue Oct 1, 2021 · 7 comments
Assignees
Labels
help wanted Issues identified as good community contribution opportunities javascript JavaScript support issues *out-of-scope Posted issue is not in scope of VS Code typescript Typescript support issues

Comments

@merceyz
Copy link

merceyz commented Oct 1, 2021

Issue Type: Bug

The in-memory file path parsing changes introduced in 8731be5 causes file paths that would have been parsed successfully by vscode.Uri.parse to no longer match. The Uri is now required to start with a slash and use a slash to seperate the schema from the path which is unexpected and seems like a bug

-^zip:/c:/foo/bar.zip/package.json
+^/zip//c:/foo/bar.zip/package.json

VS Code version: Code - Insiders 1.61.0-insider (c9e8266, 2021-10-01T14:45:54.159Z)
OS version: Windows_NT x64 10.0.19042
Restricted Mode: No

@mjbvz
Copy link
Collaborator

mjbvz commented Oct 4, 2021

What problem does this cause?

@mjbvz mjbvz added the info-needed Issue requires more information from poster label Oct 4, 2021
@merceyz
Copy link
Author

merceyz commented Oct 4, 2021

Go to Definition is broken for tsserver instances that return in-memory schemas, in our case Yarn PnP / #75559. I can update our tsserver to return paths that work with the changes but it's odd a valid vscode.Uri isn't valid anymore.

@mjbvz
Copy link
Collaborator

mjbvz commented Oct 4, 2021

Thanks for the details

Yes I recommend changing it on the plugin side. These paths are used internally by TS Server, so they don't follow vscode.URI and we don't make any guarantees about their format. That said, if you have a proposal on how to change the format in a way that won't cause regressions to other features, I'd be happy to merge that in

@mjbvz mjbvz added javascript JavaScript support issues typescript Typescript support issues and removed info-needed Issue requires more information from poster labels Oct 4, 2021
@arcanis
Copy link

arcanis commented Oct 6, 2021

@mjbvz Why was the old vscode.Uri.parse(filepath.slice(1)) incorrect? The commit message mentions errors in untitled files, but the format change doesn't seem directly connected to this, is it? Or is there another place where this new format is necessary?

@mjbvz
Copy link
Collaborator

mjbvz commented Oct 6, 2021

The change gets TS to see in-memory files as part of the same project, instead of treating them as individual units

@mjbvz mjbvz added the help wanted Issues identified as good community contribution opportunities label Oct 7, 2021
@arcanis
Copy link

arcanis commented Oct 8, 2021

Working on this, I noticed something curious, perhaps a bug:

  • Here, you craft the in-memory url string. If resource.path doesn't start with a / you add one, otherwise you keep it as is. In other words, the result is ^/zip/Users/name/project or /zip/c:/project, with a single slash after zip.
  • Here, you pattern-match against what I suspect is the same path format as above, except that you retrieve everything after the first slash and pass it to Uri.parse.

The result seems to be that the regexp will turn:

  • ^/zip/Users/name/project into Uri.parse('zip:Users/name/project')
  • ^/zip/c:/project into Uri.parse('zip:c:/project')

You can see that the / is removed from the absolute Users/name/project unless toResource processes a url with two slashes after zip - going against what the normalizedPath function does.

Is this disparity expected? It's unclear whether the expected format is a single slash between zip: and the path, or two.

@mjbvz mjbvz added the bug Issue identified by VS Code Team member as probable bug label Oct 11, 2021
@mjbvz
Copy link
Collaborator

mjbvz commented Nov 19, 2021

I'm closing this as we don't plan on taking any action on the VS Code side (and as noted, these paths are an internal implementation details)

Will still take a PR if someone submits a reasonable one

@mjbvz mjbvz closed this as completed Nov 19, 2021
@mjbvz mjbvz added *out-of-scope Posted issue is not in scope of VS Code and removed bug Issue identified by VS Code Team member as probable bug labels Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Issues identified as good community contribution opportunities javascript JavaScript support issues *out-of-scope Posted issue is not in scope of VS Code typescript Typescript support issues
Projects
None yet
Development

No branches or pull requests

3 participants