sindresorhus / get-stream

Get a stream as a string, Buffer, ArrayBuffer or array
MIT License
341 stars 33 forks source link

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

Closed theoludwig closed 1 year ago

theoludwig commented 1 year ago

Hello! :wave:

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. :tada:

ehmicky commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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! :pray:

theoludwig commented 1 year ago

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. :+1: