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: update lsp error message of 'relative import path' to 'use deno add' for npm/jsr packages #24524

Merged
merged 15 commits into from
Jul 24, 2024

Conversation

HasanAlrimawi
Copy link
Contributor

@HasanAlrimawi HasanAlrimawi commented Jul 11, 2024

This PR addresses #24033

Problem
LSP shows a message saying that the relative import path isn't prefixed with / or ./ or ../, while the package could be from npm or jsr.

Fix
This type of message arises when a Resolution error is received within the fn to_lsp_diagnostic in cli/lsp/diagnostics.rs.
Then a check is needed to see if the package name starts with @ in order to know if it's local or from npm/jsr.
After that the message can be updated to tell the user to use deno add in order to add the dependency.

Screenshot of the fix
deno lsp  deno add

@HasanAlrimawi
Copy link
Contributor Author

Hello @lucacasonato ,
Could you please review this PR when you have time? Thanks in advance.

@bartlomieju
Copy link
Member

@nayeemrmn PTAL

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.

Does this work when there's no import map? Also needs a test.

if let ResolveError::Other(resolve_error, ..) = (*error).as_ref() {
if let Some(ImportMapError::UnmappedBareSpecifier(specifier, _)) = resolve_error.downcast_ref::<ImportMapError>() {
if specifier.chars().next().unwrap_or('\0') == '@'{
message = format!("Use [deno add {}] to add the dependency.", specifier);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the hint should be appended to the original message here.

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.

LGTM

@nayeemrmn nayeemrmn merged commit fcd9bbe into denoland:main Jul 24, 2024
17 checks passed
dsherret pushed a commit that referenced this pull request Jul 26, 2024
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.

None yet

3 participants