-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Conversation
#[test] | ||
#[timeout(300_000)] | ||
fn lsp_code_actions_deno_types_for_npm() { |
There was a problem hiding this comment.
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
Line 3728 in 772fffe
new_deno_types_specifier: Option<String>, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
deno/tests/integration/lsp_tests.rs
Lines 8517 to 8519 in d969c59
#[test] | |
#[timeout(300_000)] | |
fn lsp_npm_auto_import_with_deno_types() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
d969c59
to
cd8fbab
Compare
What does that mean exactly? If I already had the |
@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: However once you update it to 2.2.1 you get: |
@nathanwhit you are the best, thank you so much! |
Makes total sense. Thanks a ton! |
Yup that's correct. I'm separately planning on a diagnostic + quick fix in the LSP for when you don't have the |
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 havewe would choose
@types/[email protected]
when resolving types forfoo
.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 forfoo
.