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: set types property in package.json for TypeScript #107

Closed
wants to merge 1 commit into from
Closed

fix: set types property in package.json for TypeScript #107

wants to merge 1 commit into from

Conversation

theoludwig
Copy link

Hello! 👋

Tried upgrading this package from v7.0.1 to 8.0.1, but using TypeScript, the types doesn't work anymore.

error TS2307: Cannot find module 'get-stream' or its corresponding type declarations.

import getStream from 'get-stream'

2 solutions exist to fix this issue:

  1. Update tsconfig.json to use "compilerOptions.moduleResolution": "NodeNext", ("NodeNext" instead of "ESNext")
  2. Update this package to add the types property in package.json

The first solution doesn't work in every cases, not all project should have "moduleResolution" to use "NodeNext", while the second solution make it so that it works for everyone.

Thank you for this v8 release, there are amazing improvements. 🎉

@ehmicky
Copy link
Collaborator

ehmicky commented Aug 21, 2023

Thanks for the kind words @theoludwig, I'm glad you find the improvements helpful!

Yes, I agree that we cannot expect everyone to use nodenext, especially if the fix is small and simple. Thanks for submitting this fix. @sindresorhus What do you think?

@sindresorhus
Copy link
Owner

I disagree. It is expected that you use at minimum Node16 (doesn't have to be NodeNext) since this package is ESM and targets Node.js 16 (and hence your project too). Adding types here just papers over the real problem of having an incorrect tsconfig.

@ehmicky ehmicky self-requested a review August 22, 2023 00:28
@ehmicky
Copy link
Collaborator

ehmicky commented Aug 22, 2023

@theoludwig One thing to keep in mind is that @sindresorhus has quite many (that's an understatement :) ) of packages, so trying to be consistent between all of them simplifies maintaining them a lot. I think the current pattern of requiring node16/nodenext for TypeScript users might be a more general requirements for the other packages too.

I hope you understand! 🙏

@theoludwig
Copy link
Author

Okay, thanks for the clarifications.
I understand the desire of being consistent, even though I still think, it should be better to support both ESNext and NodeNext use cases, especially since it's a small patch.

Closing this PR as not wanted. 👍

@theoludwig theoludwig closed this Aug 22, 2023
@theoludwig theoludwig deleted the fix-types branch August 22, 2023 18:40
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.

3 participants