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

fix(lsp): decoding percent-encoding(non-ASCII) file path correctly #22582

Merged
merged 8 commits into from
Mar 27, 2024

Conversation

Hajime-san
Copy link
Contributor

Summary

The Url alias of ModuleSpecifier type in deno_core keeps local file path with percent-encoding string while it includes non-ASCII character like emoji.
It causes unintentional completion, hover text, rename and diagnostic with percent-encoding string by lsp server.

This patch is an ad hoc approach for resolve this problem.

Expected behaviour

2024-02-26-01-c.mov

related issues

resolves #12065
resolves denoland/vscode_deno#517

There are test reporter and runtime problems.
#18983

There is a test reporter problem.
#10810

There are tsc problems.
#16058
microsoft/TypeScript#57539

Comment on lines 651 to 653
let format = |scheme: &str, rest: &str| -> String {
format!("{}​{}", scheme, rest).replace('@', "​@")
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, I'm not clear to put in zero-width space👀

@Hajime-san
Copy link
Contributor Author

Oh, jsr is invalid about unicode file path...🦕

@aapoalas aapoalas requested a review from dsherret March 5, 2024 18:40
Copy link
Collaborator

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

Conflict with main: to_hover_text has been removed in main apparently?

@Hajime-san Hajime-san force-pushed the fix-lsp-filepath-percent-encoding branch from d7d4c43 to eea5b99 Compare March 6, 2024 02:12
@Hajime-san
Copy link
Contributor Author

Fix with rebase!

@Hajime-san Hajime-san requested a review from aapoalas March 6, 2024 02:25
@Hajime-san Hajime-san force-pushed the fix-lsp-filepath-percent-encoding branch from eea5b99 to eee88d5 Compare March 8, 2024 11:33
@dsherret dsherret requested a review from nayeemrmn March 8, 2024 15:00
Copy link
Collaborator

@nayeemrmn nayeemrmn left a comment

Choose a reason for hiding this comment

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

Thanks! Essentially this patch formats 'pretty' percent-decoded URLs for the purpose of error messages, auto-import and auto-renamed specifiers. But these URLs will normalise to the same thing.

cli/lsp/tsc.rs Outdated Show resolved Hide resolved
Co-authored-by: Nayeem Rahman <nayeemrmn99@gmail.com>
Signed-off-by: Hajime-san <41257923+Hajime-san@users.noreply.github.com>
@Hajime-san
Copy link
Contributor Author

Thanks!
If this PR merged, I'll tackle to fix test reporter path correctly with the to_percent_decoded_str function in cli/util/path.rs

Hajime-san and others added 2 commits March 27, 2024 19:30
Signed-off-by: Hajime-san <41257923+Hajime-san@users.noreply.github.com>
@nayeemrmn nayeemrmn merged commit feb744c into denoland:main Mar 27, 2024
17 checks passed
@Hajime-san Hajime-san deleted the fix-lsp-filepath-percent-encoding branch March 28, 2024 01:43
bartlomieju pushed a commit that referenced this pull request May 28, 2024
…ly (#23200)

# Summary

This PR resolves about the issue.
fixes #10810

And the formerly context is in the PR. 
#22582

Here is an expected behaviour example with this change.

- 🦕.test.ts
```ts
import { assertEquals } from "https://deno.land/std@0.215.0/assert/mod.ts";

Deno.test("example test", () => {
  assertEquals("🍋", "🦕");
});
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lsp: consider handling unicode better Non-English file names cause the debugger to choke
4 participants