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(check/lsp): fall back to @types/* packages if npm package doesn't have types #28185

Merged
merged 10 commits into from
Feb 19, 2025

Conversation

nathanwhit
Copy link
Member

@nathanwhit nathanwhit commented Feb 19, 2025

Fixes #27569.
Fixes #27215.

This PR makes it so type resolution falls back to looking for definitely typed packages (@types/foo) if a given NPM package does not contain type declarations.

One complication is choosing which version of the @types/* package to use, if the project depends on multiple versions. The heuristic here is to try to match the major and minor versions, falling back to the latest version. So if you have

@types/foo: 0.1.0, 0.2.0, 3.1.0, 3.1.2, 4.0.0
foo: 3.1.0

we would choose @types/[email protected] when resolving types for foo.


Note that this only uses @types/ packages if you already depend on them. So a follow up to this PR could be to add a diagnostic and quickfix to install @types/foo if we don't find types for foo.

Comment on lines -6496 to -6498
#[test]
#[timeout(300_000)]
fn lsp_code_actions_deno_types_for_npm() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this behaviour gone? Makes sense we don't need it anymore. If so the implementation revolving around

new_deno_types_specifier: Option<String>,
should be removed

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it doesn't trigger anymore because it just uses the @types package automatically. I'll remove that bit

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess that snippet was more relevant to

#[test]
#[timeout(300_000)]
fn lsp_npm_auto_import_with_deno_types() {
. It should be unsupported for auto-imports as well I think.

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

@nathanwhit nathanwhit enabled auto-merge (squash) February 19, 2025 23:16
@nathanwhit nathanwhit merged commit 08f5e79 into denoland:main Feb 19, 2025
17 checks passed
@nathanwhit nathanwhit deleted the resolve-at-types branch February 19, 2025 23:57
@felipecrs
Copy link

Note that this only uses @types/ packages if you already depend on them.

What does that mean exactly? If I already had the @types/ package as dependency, then why would I need this feature?

@sant123
Copy link

sant123 commented Feb 21, 2025

@felipecrs with this deno.json:

{
  "imports": {
    "@types/react": "npm:@types/react@^19.0.10",
    "react": "npm:react@^19.0.0"
  }
}

And prior version 2.2.1 types will not work in LSP:

image

However once you update it to 2.2.1 you get:

image

@sant123
Copy link

sant123 commented Feb 21, 2025

@nathanwhit you are the best, thank you so much!

@felipecrs
Copy link

Makes total sense. Thanks a ton!

@nathanwhit
Copy link
Member Author

Yup that's correct. I'm separately planning on a diagnostic + quick fix in the LSP for when you don't have the @types package installed (#28216)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants