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.

Sign up for GitHub

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

Jump to bottom

refactor: rename deno specifiers to internal #17655

Merged
merged 2 commits into from
Feb 5, 2023
Merged

refactor: rename deno specifiers to internal #17655

merged 2 commits into from
Feb 5, 2023

Conversation

crowlKats
Copy link
Member

Ahead of work of adding deno specifiers to prevent issues & confusion, and for #17648

bartlomieju
bartlomieju reviewed Feb 5, 2023
View reviewed changes
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

Seems fine to me, @dsherret any objections?

dsherret
dsherret approved these changes Feb 5, 2023
View reviewed changes
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM

@crowlKats crowlKats merged commit 84a9611 into denoland:main Feb 5, 2023
@crowlKats crowlKats deleted the deno_to_internal_specifiers branch February 5, 2023 16:49
@sigmaSd
Copy link
Contributor

sigmaSd commented Feb 6, 2023

lsp clients needs to be updated, go_to_def response for example now returns internal: instead of deno:

dsherret
dsherret reviewed Feb 6, 2023
View reviewed changes
cli/lsp/language_server.rs
@@ -3121,7 +3121,7 @@ impl Inner {
.performance
.mark("virtual_text_document", Some(&params));
let specifier = self.url_map.normalize_url(&params.text_document.uri);
let contents = if specifier.as_str() == "deno:/status.md" {
let contents = if specifier.as_str() == "internal:/status.md" {
Copy link
Member

Choose a reason for hiding this comment

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

This breaks vscode_deno, which does a request to this.

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/denoland/vscode_deno/blob/4b2787824f03879e2cbbff47c9921110ce1f20fb/client/src/commands.ts#L254

@dsherret
Copy link
Member

dsherret commented Feb 6, 2023

@sigmaSd the only thing I could find in some other clients was #17655 (comment) -- do you know of anything else?

@sigmaSd
Copy link
Contributor

sigmaSd commented Feb 6, 2023 •
edited
Loading

go to definition as well fails and I assume go to refenrece and type refernece, their answer uses the deno schema

[DEBUG][2023-02-06 21:04:41] .../lua/vim/lsp.lua:1381	"LSP[denols]"	"client.request"	1	"textDocument/definition"	{  position = {    character = 57,    line = 3  },  textDocument = {    uri = "file:///home/mrcool/dev/deno/blog/main.tsx"  }}	<function 1>	7
[DEBUG][2023-02-06 21:04:41] .../vim/lsp/rpc.lua:285	"rpc.send"	{  id = 155,  jsonrpc = "2.0",  method = "textDocument/definition",  params = {    position = {      character = 57,      line = 3    },    textDocument = {      uri = "file:///home/mrcool/dev/deno/blog/main.tsx"    }  }}
[DEBUG][2023-02-06 21:04:41] .../vim/lsp/rpc.lua:388	"rpc.receive"	{  id = 155,  jsonrpc = "2.0",  result = { {      targetRange = {        ["end"] = {          character = 0,          line = 906        },        start = {          character = 0,          line = 12        }      },      targetSelectionRange = {        ["end"] = {          character = 0,          line = 906        },        start = {          character = 0,          line = 12        }      },      targetUri = "internal:/https/deno.land/std/testing/asserts.ts"    } }}

@dsherret
Copy link
Member

dsherret commented Feb 6, 2023

It's broken in vscode too. cc @crowlKats -- maybe we should revert this for now

@crowlKats crowlKats mentioned this pull request Feb 6, 2023
Merged
crowlKats added a commit that referenced this pull request Feb 6, 2023
@crowlKats
fix: revert lsp related internal specifiers (#17673)
7f23063 
To fix reports of breakage from #17655
bartlomieju pushed a commit that referenced this pull request Feb 6, 2023
@crowlKats @bartlomieju
refactor: rename deno specifiers to internal (#17655)
89c4bdf
bartlomieju pushed a commit that referenced this pull request Feb 6, 2023
@crowlKats @bartlomieju
fix: revert lsp related internal specifiers (#17673)
8110354 
To fix reports of breakage from #17655
@ayame113
Copy link
Contributor

ayame113 commented Feb 7, 2023

atom-ide-deno will also break, but it's no big deal since Atom is no longer in development. :(
https://github.com/ayame113/atom-ide-deno/blob/22e905715dd15a6dbf5cd0c641064306e4371e78/lib/virtual_documet.ts#L14

@dsherret
Copy link
Member

dsherret commented Feb 7, 2023

Thanks. It was reverted in #17673

@ayame113
Copy link
Contributor

ayame113 commented Feb 7, 2023

Glad to hear that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bartlomieju bartlomieju bartlomieju left review comments

@dsherret dsherret dsherret approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@crowlKats @sigmaSd @dsherret @ayame113 @bartlomieju