Skip to content

npm: automatic acquisition of @types package for type checking #20455

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

Closed
Tracked by #15960
nayeemrmn opened this issue Sep 11, 2023 · 8 comments · Fixed by #23419
Closed
Tracked by #15960

npm: automatic acquisition of @types package for type checking #20455

nayeemrmn opened this issue Sep 11, 2023 · 8 comments · Fixed by #23419
Labels
declined thank you, but respectfully declined node compat

Comments

@nayeemrmn
Copy link
Contributor

nayeemrmn commented Sep 11, 2023

This was already on the roadmap but with no issue.

You can usually work around this with // @deno-types="npm:@types/<package>" but that's not always an option. E.g. for "jsxImportSource": "npm:react" in your compiler options, there's no import to add the directive to. So it's a big obstacle for react-jsx users.

@nayeemrmn nayeemrmn mentioned this issue Sep 11, 2023
25 tasks
@nayeemrmn nayeemrmn changed the title automatic acquisition of @types package for type checking npm: automatic acquisition of @types package for type checking Sep 11, 2023
@dsherret
Copy link
Member

dsherret commented Sep 11, 2023

I don't think we should do this.

  1. It's magical—the types package version isn't specified.
  2. It's extremely common for the version of a @types/<package-name> package to not align with the actual package version.
  3. Figuring out if we need to do automatic type acquisition will make npm resolution slower as we need to analyze the package for if it has types (which can only be done once the tarball has been downloaded and extracted), check if there's a @types/<package-name> for that version or approxiamte version (which could be wrong), and then finally fill in that association to deno_graph (or alternatively we could require downloading and extracting all top level packages before we complete the initial deno_graph resolution).

@KyleJune
Copy link
Contributor

Not being able to use npm specifiers for jsxImportSource makes using react pretty painful in Deno. Anyone that wants to use it basically has to use esm.sh still because of this issue. See #18203 for an example.

The current hack to get react types is pretty ugly and not obvious, the only reason I know about it is from finding that issue linked above. I don't have this problem with esm.sh and thought the point of npm specifiers was so that we wouldn't need esm.sh.

// See: https://github.com/denoland/deno/issues/16653
import type {} from "npm:@types/react@18";
import type {} from "npm:@types/react-dom@18";

The other option of adding a comment is a bit painful too. I don't think you need to do anything like that to get types for your npm dependencies in node.

// @deno-types="npm:@types/<package>"

It would be great if we could just import react and get the types we would expect to get if we did the same in node.

@dsherret
Copy link
Member

dsherret commented Sep 12, 2023

@KyleJune I think we need a different solution for that then this. For example, this won't help if there is a version mismatch. Let's discuss in the other issue.

@oscarotero
Copy link
Contributor

I think the main problem is not if the option is ugly or not, but there's no way to specify this globally. Maybe an option in deno.json to assign types to any module? Something like:

{
  "compilerOptions": {
    "jsx": "react-jsx",
    "jsxImportSource": "npm:react",
  }, 
  "types": {
    "npm:react": "npm:@types/react@18"
  }
}

@dsherret
Copy link
Member

@oscarotero yes, see #18203 (comment) and below

@sant123
Copy link

sant123 commented May 1, 2024

@dsherret should this work for any package right? I'm testing with mssql but I still need to specify the @deno-types comment.

With comment:
image

Without comment:
image

@dsherret dsherret reopened this May 1, 2024
@dsherret
Copy link
Member

dsherret commented May 1, 2024

Sorry, this issue shouldn't have been closed as part of jsxImportSourceTypes support. That said, I'm pretty sure this issue won't ever be done for the reasons I outlined above and so I'm going to close this as not planned.

@dsherret dsherret closed this as not planned Won't fix, can't repro, duplicate, stale May 1, 2024
@dsherret dsherret added the declined thank you, but respectfully declined label May 1, 2024
@KyleJune
Copy link
Contributor

KyleJune commented Sep 1, 2024

Sorry, this issue shouldn't have been closed as part of jsxImportSourceTypes support. That said, I'm pretty sure this issue won't ever be done for the reasons I outlined above and so I'm going to close this as not planned.

Is there a way or a plan for a way to specify types for packages like the mssql package referenced in a previous comment other than adding a deno types comment everywhere you import it? I believe in node you can get it to acquire types by installing the @types/mssql package which adds an entry to your package.json file. Will that work in deno? If so, do we need a package.json if an npm package we depend on needs types or is there another way to get them in the deno.json file?

Edit: The best workaround I know of currently is adding .d.ts files for any npm packages missing types. For example, a react.d.ts file like this:

declare module "react" {
  // @ts-types="@types/react"
  import React from "npm:react@18";
  export = React;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
declined thank you, but respectfully declined node compat
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants