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
feat(@types/node): add stream/consumers
#55311
feat(@types/node): add stream/consumers
#55311
Conversation
@favna Thank you for submitting this PR! This is a live comment which I will keep updated. 1 package in this PR
Code ReviewsBecause this is a widely-used package, a DT maintainer will need to review it before it can be merged. You can test the changes of this PR in the Playground. Status
All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 55311,
"author": "Favna",
"headCommitOid": "028811c99b8fa33b2181f725639a385ded81bb29",
"lastPushDate": "2021-08-26T10:41:33.000Z",
"lastActivityDate": "2021-08-27T18:00:33.000Z",
"mergeOfferDate": "2021-08-27T09:32:42.000Z",
"mergeRequestDate": "2021-08-27T18:00:33.000Z",
"mergeRequestUser": "Favna",
"hasMergeConflict": false,
"isFirstContribution": false,
"tooManyFiles": false,
"popularityLevel": "Critical",
"pkgInfo": [
{
"name": "node",
"kind": "edit",
"files": [
{
"path": "types/node/index.d.ts",
"kind": "definition"
},
{
"path": "types/node/stream.d.ts",
"kind": "definition"
},
{
"path": "types/node/stream/consumers.d.ts",
"kind": "definition"
},
{
"path": "types/node/test/stream.ts",
"kind": "test"
}
],
"owners": [
"Microsoft",
"DefinitelyTyped",
"jkomyno",
"alvis",
"r3nya",
"btoueg",
"smac89",
"touffy",
"DeividasBakanas",
"eyqs",
"Hannes-Magnusson-CK",
"KSXGitHub",
"hoo29",
"kjin",
"ajafff",
"islishude",
"mwiktorczyk",
"mohsen1",
"n-e",
"galkin",
"parambirs",
"eps1lon",
"SimonSchick",
"ThomasdenH",
"WilcoBakker",
"wwwy3y3",
"samuela",
"kuehlein",
"bhongy",
"chyzwar",
"trivikr",
"nguymin4",
"yoursunny",
"qwelias",
"ExE-Boss",
"Ryan-Willpower",
"peterblazejewicz",
"addaleax",
"JasonHK",
"victorperin",
"ZYSzys",
"NodeJS",
"LinusU"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Critical"
}
],
"reviews": [
{
"type": "approved",
"reviewer": "peterblazejewicz",
"date": "2021-08-27T09:32:06.000Z",
"isMaintainer": true
},
{
"type": "approved",
"reviewer": "LinusU",
"date": "2021-08-26T15:18:50.000Z",
"isMaintainer": false
}
],
"mainBotCommentID": 903333595,
"ciResult": "pass"
} |
🔔 @microsoft @DefinitelyTyped @jkomyno @alvis @r3nya @btoueg @smac89 @Touffy @DeividasBakanas @eyqs @Hannes-Magnusson-CK @KSXGitHub @hoo29 @kjin @ajafff @islishude @mwiktorczyk @mohsen1 @n-e @galkin @parambirs @eps1lon @SimonSchick @ThomasdenH @WilcoBakker @wwwy3y3 @samuela @kuehlein @bhongy @chyzwar @trivikr @nguymin4 @yoursunny @qwelias @ExE-Boss @Ryan-Willpower @peterblazejewicz @addaleax @JasonHK @victorperin @ZYSzys @nodejs @LinusU — please review this PR in the next few days. Be sure to explicitly select |
@favna The CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review. |
@favna The CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review. |
This comment has been minimized.
This comment has been minimized.
@favna The CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review. |
ts3.6 is being phased out, see #54895 You can safely ignore these errors and we can proceed once this PR is submitted. |
This comment has been minimized.
This comment has been minimized.
I'd revert them as you won't really have to do any of it. |
f1a9830
to
e624d76
Compare
@favna Unfortunately, this pull request currently has a merge conflict 😥. Please update your PR branch to be up-to-date with respect to master. Have a nice day! |
This is a set of new functions added in Node 16.7. As the types are now 16.7.1, we can add them. NodeJS docs: https://nodejs.org/dist/latest-v16.x/docs/api/webstreams.html#webstreams_utility_consumers
e624d76
to
028811c
Compare
@SimonSchick PR has been rebased on top of master. Thanks for your contributions in your PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! 👍
Ready to merge |
I just published |
// This interface is not, and should not be, exported. | ||
interface Blob { | ||
readonly size: number; | ||
readonly type: string; | ||
arrayBuffer(): Promise<ArrayBuffer>; | ||
slice(start?: number, end?: number, contentType?: string): Blob; | ||
stream(): NodeJS.ReadableStream; | ||
text(): Promise<string>; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this comment is accurate? It looks like this interface is leaking into global definitions:
I can't remember where I read it (maybe in a linter rule or something?), but I'm pretty sure all interfaces in a .d.ts
file are exported, so that consumers have access to references to them. Very happy to be proven wrong on this point, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not only is that comment (about not being exported) inaccurate, the comment above it isn't accurate either. The interface is not a "duplicate" of Blob from lib.dom.ts
, because the DOM Blob#stream
method returns a W3C ReadableStream, which is not the same thing as a NodeJS.ReadableStream
.
As you point out, simply having @types/node
in your node_modules
pollutes frontend code with this Node-specific global type definition, which is merged with the real (DOM) Blob definition. As a result, I've had to any-cast the return value whenever I call .stream()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good news, the correct types have been added in the interim so this erroneous "duplicate" is no longer needed. I just opened #59905 which I think will resolve this error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thw0rted With the benefit of the doubt I will think you didn't mean it as such but I feel called out here anyway, and so I feel a need to defend myself.
I specifically did not add myself to the contributors list for node types because I have no interest in being pinged for reviews and I have hardly any interest in Node's types to begin with as long as they work, and this has for many months.
Furthermore, even though yes it might be wrong that I made this change, that change was made in a pull request just like your own. That means it was reviewed and it was merged by a DT maintainer. Simply denotsting that I did bad here is therefore coming over as shallow because clearly other people were involved as well.
Lastly, as I have also stated elsewhere, this review comment was made after the pull request was already merged. It was therefore quite obviously never taken into full consideration and mostly completely dismissed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right that I didn't mean it as a personal attack -- reading back through my comment I can definitely see how you'd take it that way, and I'm sorry. (I can sympathize about not signing yourself up as a contributor, the scope is unlike anything else on DT!)
The situation is very confusing, and I don't know that anybody can really be blamed directly -- Node chose to add this partial implementation of some web APIs, and worse they're reusing web class names for incomplete implementations that aren't available at the same scope. When you made your PR you didn't even have a "Node Blob" to reference, so your approach was totally reasonable; bleeding into global was an honest mistake.
To make matters worse, I don't see any way to write a test (or I guess a "meta" test?) that would prevent this. Maybe the package.json or tsconfig file could have a field with a list of globals introduced by the package, and if anything else bleeds out, the test would fail? I don't really know enough about TS internals to suggest how to implement something like that but it might be worth kicking the idea around in Discussions -- I'll take a look.
At the moment, some users' builds fail, because `Blob` isn't defined as a global in Node.js - it sits in the [`buffer`][1] package. This change adds an import from `buffer` to fix this issue. Note that I think the build was incorrectly passing before because of a [leaky interface in `stream/consumers`][2]. [1]: https://nodejs.org/api/buffer.html#class-blob [2]: DefinitelyTyped#55311 (comment)
At the moment, some users' builds fail, because `Blob` isn't defined as a global in Node.js - it sits in the [`buffer`][1] package. This change adds an import from `buffer` to fix this issue. Note that I think the build was incorrectly passing before because of a [leaky interface in `stream/consumers`][2]. [1]: https://nodejs.org/api/buffer.html#class-blob [2]: DefinitelyTyped#55311 (comment)
At the moment, some users' builds fail, because `Blob` isn't defined as a global in Node.js - it sits in the [`buffer`][1] package. This change adds an import from `buffer` to fix this issue. Note that I think the build was incorrectly passing before because of a [leaky interface in `stream/consumers`][2]. Note that I've had to disable the linter's `no-outside-dependencies` because of a [known false positive on `buffer`][3]. [1]: https://nodejs.org/api/buffer.html#class-blob [2]: DefinitelyTyped#55311 (comment) [3]: microsoft/dtslint#315
At the moment, some users' builds fail, because `Blob` isn't defined as a global in Node.js - it sits in the [`buffer`][1] package. This change adds an import from `buffer` to fix this issue. Note that I think the build was incorrectly passing before because of a [leaky interface in `stream/consumers`][2]. Note that I've had to disable the linter's `no-outside-dependencies` because of a [known false positive on `buffer`][3]. [1]: https://nodejs.org/api/buffer.html#class-blob [2]: DefinitelyTyped#55311 (comment) [3]: microsoft/dtslint#315
At the moment, some users' builds fail, because `Blob` isn't defined as a global in Node.js - it sits in the [`buffer`][1] package. This change adds an import from `buffer` to fix this issue. Note that I think the build was incorrectly passing before because of a [leaky interface in `stream/consumers`][2]. Note that I've had to disable the linter's `no-outside-dependencies` because of a [known false positive on `buffer`][3]. [1]: https://nodejs.org/api/buffer.html#class-blob [2]: #55311 (comment) [3]: microsoft/dtslint#315
At the moment, some users' builds fail, because `Blob` isn't defined as a global in Node.js - it sits in the [`buffer`][1] package. This change adds an import from `buffer` to fix this issue. Note that I think the build was incorrectly passing before because of a [leaky interface in `stream/consumers`][2]. Note that I've had to disable the linter's `no-outside-dependencies` because of a [known false positive on `buffer`][3]. [1]: https://nodejs.org/api/buffer.html#class-blob [2]: DefinitelyTyped#55311 (comment) [3]: microsoft/dtslint#315
This is a set of new functions added in Node 16.7. As the types are now 16.7.x, we can add them.
NodeJS docs: https://nodejs.org/dist/latest-v16.x/docs/api/webstreams.html#webstreams_utility_consumers
Please fill in this template.
npm test <package to test>
.Select one of these and delete the others:
If changing an existing definition:
If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.