feat(lsp): add registry import auto-complete #9934

kitsonk · 2021-03-30T11:34:18Z

Still have a few things to do, but it is in enough of a shape to share.

This add the feature where by LSP clients can specify hosts where the LSP will attempt to retrieve a meta data file which describes how to create remote module specifier URLs for modules located on the server, and allows fetching of the completion items.

lucacasonato

I have taken it for a spin locally. Some feedback:

The sorting of modules is not great. When searching for https://deno.land/x/s3 these are the results:
. The module that is exactly named s3 is 5th place.
Completing on the final / in https://deno.land/x/s3@0.4.0/src/ will complete remote imports, not registry imports.
Completing on the final d in https://deno.land/x/s3@0.4.0/src/d will complete all paths, not just ones starting with /src/d
Accepting the deps.ts completion on https://deno.land/x/s3@0.4.0/src/d will insert https://deno.land/x/s3@0.4.0/src/ddeps.ts.
Completing in the middle of a specifier will set the cursor all the way to the end of the specifier.

Regarding the deno.suggest.imports.reloadCache option: The v2 branch had http caching, and had a deno.resetRegistryCache command. Maybe we could introduce that command instead of deno.suggest.imports.reloadCache until #9931 lands?

Another remark: the autoDiscovery option seems to work differently in this version than it did in v2. In v2 when autoDiscovery was enabled, we would try fetch the manifest when we see it for the first time. We would do that at max every 24 hours. If we found a manifest we would prompt the user if they wanted completions from this registry or not. We would persist their response in the hosts setting. In this version it seems autoDiscovery is not yet present. Maybe we should just disable that option for now?

Anyway, the core works and that is fantastic. It feels nice and snappy. Great feature to have :-)

lucacasonato · 2021-03-30T11:59:39Z

cli/lsp/registries.rs

+use std::collections::HashSet;
+use std::path::Path;
+
+const CONFIG_PATH: &str = "/.well-known/deno-import-intellisense.json";


Suggested change

const CONFIG_PATH: &str = "/.well-known/deno-import-intellisense.json";

const CONFIG_PATH: &str = "/.well-known/deno-import-completions.json";

We should consider renaming this. IntelliSense is specifically a Microsoft term I think.

I would prefer to land it without requiring existing registry providers to change anything. It is a Microsoft specific term, but it is a non-user facing bit of information, so... 🤷

As far as I know it is just deno.land/x and x.lcas.dev that have it set up right now. I would rather fix it now than keeping the cruft around forever.

kitsonk · 2021-03-30T23:56:23Z

The sorting of modules is not great. When searching for https://deno.land/x/s3 these are the results:

That is entirely a vscode thing. We can provide alternative sort text, but it is vscode that does it. There is a little interaction with what text you have typed during the completions that vscode is matching on, I will see if we can "clear" that before a commit char.

Completing on the final / in https://deno.land/x/s3@0.4.0/src/ will complete remote imports, not registry imports.

That is a bug, will look into that. I am having to restructure some of the path_to_regex stuff anyways that would allow us to complete each part of a path, part by part as well.

Completing on the final d in https://deno.land/x/s3@0.4.0/src/d will complete all paths, not just ones starting with

Ok, another bug... I think breaking the parts into components will help with that too...

Regarding the deno.suggest.imports.reloadCache option: The v2 branch had http caching, and had a deno.resetRegistryCache command. Maybe we could introduce that command instead of deno.suggest.imports.reloadCache until #9931 lands?

I thought about that, the problem is that it isn't a "command" in the sense that we don't clear the cache, we change the cache setting in the file fetcher from ::Use to ::ReloadAll, so it only reloads when it tries to load something again. The command would need to actually try to remove the /registries path from the DENO_DIR and that starts to get into all sorts of potential local file system permission issues maybe.

Another remark: the autoDiscovery option...

Yeah, I didn't understand that, let's get rid of it for now.

kitsonk · 2021-04-01T04:22:57Z

Ok, I think I fixed most of the filtering/sorting issues and other stuff...

lucacasonato · 2021-04-01T09:26:57Z

Just tried it out again, and feels pretty damn great now. Just some minor comments:

Versions are listed out of order (oldest first) - the order returned from https://deno.land/_vsc1/modules/s3 is reversed.
Accepting a completion always resets the cursor to the end of the specifier, instead of the end of completed part. For example you complete a version, and the cursor is set to the end of the path.

I still think we should switch to a Deno: Clear import completion cache instead of a the Reload cache setting. It feels really strange to use.

kitsonk · 2021-04-01T09:55:00Z

the order returned from https://deno.land/_vsc1/modules/s3 is reversed

vscode is sorting them based on the label... I think I can fix that though.

Accepting a completion always resets the cursor to the end of the specifier, instead of the end of completed part. For example you complete a version, and the cursor is set to the end of the path.

Yeah, I am not sure any good way around that because we are using text edits in the return value to make sure the specifier is proper proper.

kitsonk · 2021-04-01T10:31:55Z

vscode is sorting them based on the label... I think I can fix that though.

Ok, I can fix that, but it is an all or nothing... either initially listed in the order it is returned from the server or it is "alphabetical"... the protocol doesn't have (yet) a way to indicate what sort of sorting to apply. So when I change it to server order, some of the other fields look "odd"...

I am fine with use saying "it is up to the server" but some of return values of deno.land/x/ look odd...

lucacasonato · 2021-04-01T10:35:34Z

I would say for now the server should be authoritative regarding sorting, and later we can extend the registry manifest to decide what sorting to apply for each field.

kitsonk · 2021-04-01T10:46:05Z

Actually most of the oddness was related to me converting the index of item into a string without padding it, so "1", "2" ... "10" ... "20" were getting oddly sorted.

kitsonk · 2021-04-05T12:08:02Z

Ready for a proper review now.

lucacasonato

Implementation looks great. Tried it out again and found a few more things:

completing at the end of specifier https://deno.land/x brings up "remote" completions, not "registry" completions
completing in an empty specifier does not bring up https://deno.land as completion. completing in a specifier with just h does.

Once those are fixed this is good to land. In a follow up we should address the following:

auto discovery of registries like in v2, with user popup
respecting HTTP cache headers for completions and manifest

cli/lsp/completions.rs

cli/lsp/registries.rs

kitsonk · 2021-04-08T12:14:26Z

Ok, I think I have fixed both the issues you mentioned.

Another thing for follow up is handling auto-completions with import maps, though that is more import completions, versus registry completions.

lucacasonato · 2021-04-08T18:22:50Z

I think that last round of fixes introduced a new error: completing at the end of https://deno.land inserts std@ not /std@ resulting in https://deno.landstd@.

kitsonk · 2021-04-09T00:21:26Z

@lucacasonato ok, that is fixed... added a regression test for it too... strange, it should have been broken all along, but I have made that all a bit more robust now.

lucacasonato

L. G. T. M. 💪

Thanks for enduring this long review processes. Fantastic feature, and incredibly well implemented. Such a pleasure to use. Let's level up this LSP 🚀

lucacasonato reviewed Mar 30, 2021

View reviewed changes

kitsonk force-pushed the feat_lsp_registry_imports branch 2 times, most recently from 56a3e29 to 0dc1702 Compare April 1, 2021 04:20

kitsonk force-pushed the feat_lsp_registry_imports branch from 0c9f4a7 to 505a624 Compare April 1, 2021 08:42

kitsonk force-pushed the feat_lsp_registry_imports branch from f3166dd to 7a683b6 Compare April 1, 2021 11:03

kitsonk marked this pull request as draft April 1, 2021 23:14

kitsonk mentioned this pull request Apr 2, 2021

Deno language server to-do list #8643

Closed

43 tasks

kitsonk force-pushed the feat_lsp_registry_imports branch 2 times, most recently from cf5f10b to c9da5d2 Compare April 5, 2021 04:21

kitsonk marked this pull request as ready for review April 5, 2021 12:07

kitsonk changed the title ~~[WIP] feat(lsp): add registry import auto-complete~~ feat(lsp): add registry import auto-complete Apr 5, 2021

kitsonk requested a review from lucacasonato April 5, 2021 12:07

lucacasonato reviewed Apr 6, 2021

View reviewed changes

cli/lsp/completions.rs Outdated Show resolved Hide resolved

cli/lsp/registries.rs Outdated Show resolved Hide resolved

kitsonk force-pushed the feat_lsp_registry_imports branch 2 times, most recently from 44b3202 to a52d134 Compare April 8, 2021 12:12

kitsonk added 5 commits April 9, 2021 09:37

feat(lsp): add registry import auto-complete

fd479c9

fixups

aa60c06

fix filtering

c8bd068

clippy

921799f

moar clippy

d921ca8

kitsonk added 9 commits April 9, 2021 09:37

fmt

36d998f

fix sorting

e57307c

remove config options, add reload command

0a3b92a

fix existing tests

d31c7ff

fix more tests

8433e29

refactors and integration test

0d71182

address feedback

ffc979a

fix completions in certain scenarios

773ddfe

fix token 0 completions

eaec5a9

kitsonk force-pushed the feat_lsp_registry_imports branch from a52d134 to eaec5a9 Compare April 9, 2021 00:19

lucacasonato approved these changes Apr 9, 2021

View reviewed changes

kitsonk merged commit d9d4a5d into denoland:main Apr 9, 2021

kitsonk deleted the feat_lsp_registry_imports branch April 9, 2021 01:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(lsp): add registry import auto-complete #9934

feat(lsp): add registry import auto-complete #9934

kitsonk commented Mar 30, 2021

lucacasonato left a comment

lucacasonato Mar 30, 2021

kitsonk Apr 5, 2021

lucacasonato Apr 5, 2021

kitsonk commented Mar 30, 2021

kitsonk commented Apr 1, 2021

lucacasonato commented Apr 1, 2021

kitsonk commented Apr 1, 2021

kitsonk commented Apr 1, 2021

lucacasonato commented Apr 1, 2021

kitsonk commented Apr 1, 2021

kitsonk commented Apr 5, 2021

lucacasonato left a comment

kitsonk commented Apr 8, 2021

lucacasonato commented Apr 8, 2021

kitsonk commented Apr 9, 2021

lucacasonato left a comment

	const CONFIG_PATH: &str = "/.well-known/deno-import-intellisense.json";
	const CONFIG_PATH: &str = "/.well-known/deno-import-completions.json";

feat(lsp): add registry import auto-complete #9934

feat(lsp): add registry import auto-complete #9934

Conversation

kitsonk commented Mar 30, 2021

lucacasonato left a comment

Choose a reason for hiding this comment

lucacasonato Mar 30, 2021

Choose a reason for hiding this comment

kitsonk Apr 5, 2021

Choose a reason for hiding this comment

lucacasonato Apr 5, 2021

Choose a reason for hiding this comment

kitsonk commented Mar 30, 2021

kitsonk commented Apr 1, 2021

lucacasonato commented Apr 1, 2021

kitsonk commented Apr 1, 2021

kitsonk commented Apr 1, 2021

lucacasonato commented Apr 1, 2021

kitsonk commented Apr 1, 2021

kitsonk commented Apr 5, 2021

lucacasonato left a comment

Choose a reason for hiding this comment

kitsonk commented Apr 8, 2021

lucacasonato commented Apr 8, 2021

kitsonk commented Apr 9, 2021

lucacasonato left a comment

Choose a reason for hiding this comment