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

feat(lsp): npm specifier completions #20121

Merged
merged 20 commits into from Aug 29, 2023
Merged

Conversation

nayeemrmn
Copy link
Collaborator

Closes #15811.

Snipping tool screen recorder isn't working so here's some screenshots:
image
image

@nayeemrmn
Copy link
Collaborator Author

What kind of test would we allow for this?

@ry
Copy link
Member

ry commented Aug 10, 2023

Nice work. I think there are some existing completion tests in the LSP code that could be copied for this?

@nayeemrmn
Copy link
Collaborator Author

This one depends on the npm registry site... I think I'll swap out the endpoint URLs in a cfg(test) to a localhost with fake responses.

@bartlomieju
Copy link
Member

This one depends on the npm registry site... I think I'll swap out the endpoint URLs in a cfg(test) to a localhost with fake responses.

That's a great idea.

@nayeemrmn
Copy link
Collaborator Author

Okay we already have a CliNpmRegistryApi abstraction whose origin can be overrided with an env var which npm tests use.

@nayeemrmn
Copy link
Collaborator Author

I wanted to reuse the loading stuff in CliNpmRegistryApi but when I tried to move the search API request over there was a lot of concurrency and cache machinery that looked overkill for the LSP. That means there are now two spots where package info is fetched.

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.

Thanks! This will be a big improvement.

cli/lsp/completions.rs Outdated Show resolved Hide resolved
cli/lsp/completions.rs Outdated Show resolved Hide resolved
@bartlomieju bartlomieju modified the milestones: 1.36, 1.37 Aug 16, 2023
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.

Almost there. Given the changes made that expose the high level construction code to the unit tests, I think we should make a seam for the api provided to get_npm_completions so we don't need to expose create_npm_api_and_cache.

cli/npm/registry.rs Outdated Show resolved Hide resolved
cli/lsp/language_server.rs Outdated Show resolved Hide resolved
cli/lsp/completions.rs Show resolved Hide resolved
cli/npm/registry.rs Outdated Show resolved Hide resolved
cli/lsp/registries.rs Show resolved Hide resolved
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.

Sorry for my delay on this one! I thought it was good and we were just waiting for the merge window to open, but then I just looked at it a bit more and I think it will only use the cached package information. Additionally, we could decouple get_npm_completions from deno_npm's NpmRegistryApi trait.

cli/lsp/completions.rs Show resolved Hide resolved
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.

Looks great. Nice work and thanks @nayeemrmn!

@dsherret dsherret merged commit b5f032d into denoland:main Aug 29, 2023
13 checks passed
@nayeemrmn nayeemrmn deleted the npm-completions branch August 30, 2023 18:14
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: npm registry completions on module specifiers
4 participants