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

Publish TypeScript types with package #368

Closed
dstaley opened this issue Feb 18, 2020 · 9 comments · Fixed by DefinitelyTyped/DefinitelyTyped#42438
Closed

Publish TypeScript types with package #368

dstaley opened this issue Feb 18, 2020 · 9 comments · Fixed by DefinitelyTyped/DefinitelyTyped#42438
Milestone

Comments

@dstaley
Copy link

dstaley commented Feb 18, 2020

Congratulations on releasing the big ES modules rewrite! I gave it a try in a simple toy app, and it works wonderfully. It was great to see tree-shaking working correctly!

The only feedback I have is that it'd be really nice to have the TypeScript definitions published as part of the package instead of a separate @types package. This would ensure that all consumers of v7 and up are able to use the package with TypeScript without installing another package that is potentially out of date. As an example, I'm unable to test the new v7 beta in my TypeScript projects since the published types are only for v3. In order to use the beta, I'd need to write my own declaration file, and if I'm doing that I figured I might as well offer to do it officially.

If you're interested, I'm willing to write new type declarations and send a PR adding them.

@ctavan
Copy link
Member

ctavan commented Feb 18, 2020

@dstaley thank you for taking the time to test the new version, I'm glad to hear it worked well for you!

I'm reluctant to include the types in this project as long as it is written in JS, since the Typescript docs explicitly state:

Now that you have authored a declaration file following the steps of this guide, it is time to publish it to npm. There are two main ways you can publish your declaration files to npm:

  • bundling with your npm package, or
  • publishing to the @types organization on npm.

If your package is written in TypeScript then the first approach is favored. Use the --declaration flag to generate declaration files. This way, your declarations and JavaScript will always be in sync.

If your package is not written in TypeScript then the second is the preferred approach.

Since I'm not a regular TypeScript user I don't know if packages tend to stick to these rules or not.

In any case I'm still happy to maintain types that are in-sync with the developments in this repo. In fact I have already submitted a PR over at DefinitelyTyped/DefinitelyTyped#42438 that should clean things up for v7.0.0 (once we release the stable version).

I would be really happy if you could take a look at the type definitions over there and leave your feedback.

@dstaley
Copy link
Author

dstaley commented Feb 18, 2020

@ctavan That's great to hear! Each method has its trade-offs (for instance, this exact scenario where you can't ship prerelease types), but as long as you're willing to maintain the types users will have good experiences.

In the meantime, I'll try manually installing your changes into node_modules and giving it a test in my TypeScript projects and letting you know the results.

@dstaley
Copy link
Author

dstaley commented Feb 18, 2020

@ctavan I totally forgot that npm can resolve packages via GitHub, so I pushed your type definition to a repo and installed it in my TypeScript projects. It works wonderfully, and I confirmed that the module field is correctly resolved by bundlers like rollup.

I can't wait for this to land. Thank you for all the work you've done!

For anyone else that wants to give it a try, you can run npm install https://github.com/dstaley/types-uuid.

@ibc
Copy link

ibc commented Feb 24, 2020

Right now I cannot upgrade to uuid v7 because my app/lib is written in TypeScript so it also needs @types/uuid which, right now, is still 3.4.7.

Please, consider adding TypeScript into uuid itself. If you don't want, you don't need to rewrite the whole code to TS but just add a "types": "index.d.ts" in package.json and create a index.d.ts that exposes the public library API in TypeScript. Something like this:

https://github.com/ibc/h264-profile-level-id

@ctavan
Copy link
Member

ctavan commented Feb 24, 2020

We do have a pending PR for the types at DefinitelyTyped/DefinitelyTyped#42438

It should be merged within the next few days. Please see my comment above for the reasoning why we don't bundle the types with this module at the moment.

@ibc
Copy link

ibc commented Feb 24, 2020

Yes, sorry. I just consider that it's easier to include types definition within the own package rather than publishing a separate package to @types. Obviously up to you :)

@broofa
Copy link
Member

broofa commented Feb 24, 2020

easier to include types definition within the own package

... for typescript users, not necessarily for package developers, especially ones who are not themselves TypeScript devs.

@ctavan: I'd suggest only taking a PR that adds typescript defs if the person is willing to support that them moving forward. I.e. There should be a comment-header in the file that says, "Please make sure issues related to these typedefs are assigned to @[PR author] in Github", or words to that effect.

@ctavan
Copy link
Member

ctavan commented Feb 25, 2020

@types/uuid@7.0.0 has finally been released and should fix this issue.

As mentioned above I'm happy to update the TypeScript definitions over at DefinitelyTyped if the API surface of this module changes, but I will follow the official recommendation of not bundling the type definitions into this module as long as it is not written in TypeScript.

@ctavan ctavan closed this as completed Feb 25, 2020
@ibc
Copy link

ibc commented Feb 25, 2020

Thanks, works perfectly (just tested in Node).

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 a pull request may close this issue.

4 participants