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

Add missing generic parameter to ReadableStream type #6554

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LinusU
Copy link

@LinusU LinusU commented Oct 9, 2024

This was added to the upstream dom typings in the Nov 2018 update.

ref:
microsoft/TypeScript@b830ef8
microsoft/TypeScript#28343
microsoft/TypeScript-DOM-lib-generator#541

Issue

n/a

Description

This updates the ReadableStream type to match the upstream dom.d.ts from which it was mirrored in the first place.

This is needed because otherwise it can result in errors like these:

node_modules/form-data-encoder/lib/index.d.ts:30:15 - error TS2315: Type 'ReadableStream' is not generic.

30     stream(): ReadableStream<Uint8Array> | AsyncIterable<Uint8Array>;
                 ~~~~~~~~~~~~~~~~~~~~~~~~~~


Found 1 error in node_modules/form-data-encoder/lib/index.d.ts:30

Testing

I tested this by making the change in my node_modules folder and building and testing our project.

Additional context

Add any other context about the PR here.

Checklist

  • [n/a] If you wrote E2E tests, are they resilient to concurrent I/O?
  • [n/a] If adding new public functions, did you add the @public tag and enable doc generation on the package?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

kuhe
kuhe previously approved these changes Oct 16, 2024
@kuhe kuhe dismissed their stale review October 16, 2024 21:23

needs testing

@@ -26,7 +26,7 @@ declare global {
/**
* @public
*/
export interface ReadableStream {}
export interface ReadableStream<R = any> {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this an issue if you have the dom or node types library installed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kuhe sorry for the late reply

This is how it's defined both in the DOM typings:

https://github.com/microsoft/TypeScript/blob/cb44488fcec4348a448434afbf2ebcbf2b423c61/src/lib/dom.generated.d.ts#L19086

and in the Node.js typings:

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/3c2bac796de55bd7fe6c00bab670829efa6fbc1e/types/node/stream/web.d.ts#L173

I have had no problems using it in our project that has the node typings installed, and I just tested adding DOM to libs and it works as well 👍

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.

2 participants