-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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: Blob.stream return type which is incorrect and conflicts with lib.dom.d.ts #59057
Conversation
Looks like issue was raised during review #55311 (comment) but things have landed anyhow. |
@favna I think you've introduced this definition into codebase, mind providing some context here and whether proposed changes would cause problems ? Thanks |
Looks like Again I'm not sure I understand the intricacies here, only other alternative I can think of is removing |
Looks like there is another place in node where Blob is defined, although there https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/node/buffer.d.ts#L164 I've made another change which removes copy of blob definition and refers to one from the |
@Gozala The comment you linked was raised after the PR was already approved (by DT maintainers mind you, which is required for node types) and merged, ergo "it was merged anyhow"... Doesn't really apply. Obviously we cannot fully remove stream because it is actually functionality that's in NodeJS: https://nodejs.org/dist/latest-v17.x/docs/api/stream.html I'm sure the maintainers of Node are all open to potential solutions, but I'm not one of them by choice so all of this said I will distance myself from this issue. Once you remove this PR from draft and open it for review you'll also notice that the bot won't be pinging me. |
Addition to my comment above, I recommend you undraft this PR. Right now no one really becomes aware that the PR exists. Only I did because you mentioned me. However once you open the PR for review the GitHub bot will ping all maintainers and then people can let you know what's what. |
@Gozala Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through. This is a live comment which I will keep updated. 1 package in this PRCode 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
Once every item on this list is checked, I'll ask you for permission to merge and publish the changes. InactiveThis PR has been inactive for 31 days — it is considered abandoned, and therefore closed! Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 59057,
"author": "Gozala",
"headCommitOid": "f4c3955fe966c65fe528c1963e0fa217034c6ca5",
"mergeBaseOid": "2280eb45b474bf8e92ff34b1d2775afc03cffc6f",
"lastPushDate": "2022-03-02T00:04:06.000Z",
"lastActivityDate": "2022-03-15T21:03:29.000Z",
"hasMergeConflict": false,
"isFirstContribution": true,
"tooManyFiles": false,
"hugeChange": false,
"popularityLevel": "Critical",
"pkgInfo": [
{
"name": "node",
"kind": "edit",
"files": [
{
"path": "types/node/stream/consumers.d.ts",
"kind": "definition"
}
],
"owners": [
"Microsoft",
"DefinitelyTyped",
"jkomyno",
"alvis",
"r3nya",
"btoueg",
"smac89",
"touffy",
"DeividasBakanas",
"eyqs",
"Hannes-Magnusson-CK",
"hoo29",
"kjin",
"ajafff",
"islishude",
"mwiktorczyk",
"mohsen1",
"n-e",
"galkin",
"parambirs",
"eps1lon",
"SimonSchick",
"ThomasdenH",
"WilcoBakker",
"wwwy3y3",
"samuela",
"kuehlein",
"bhongy",
"chyzwar",
"trivikr",
"yoursunny",
"qwelias",
"ExE-Boss",
"peterblazejewicz",
"addaleax",
"victorperin",
"ZYSzys",
"NodeJS",
"LinusU",
"wafuwafu13"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Critical"
}
],
"reviews": [
{
"type": "changereq",
"reviewer": "gabritto",
"date": "2022-03-15T21:03:29.000Z"
}
],
"mainBotCommentID": 1055947315,
"ciResult": "pass"
} |
🔔 @microsoft @DefinitelyTyped @jkomyno @alvis @r3nya @btoueg @smac89 @Touffy @DeividasBakanas @eyqs @Hannes-Magnusson-CK @hoo29 @kjin @ajafff @islishude @mwiktorczyk @mohsen1 @n-e @galkin @parambirs @eps1lon @SimonSchick @ThomasdenH @WilcoBakker @wwwy3y3 @samuela @kuehlein @bhongy @chyzwar @trivikr @yoursunny @qwelias @ExE-Boss @peterblazejewicz @addaleax @victorperin @ZYSzys @nodejs @LinusU @wafuwafu13 — please review this PR in the next few days. Be sure to explicitly select |
@Gozala 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 I think it came out as a blame here, but that was not my intention I was just trying to figure out the context, so I would have a chance of a fix.
I'm not sure I fully understand the relation here. I'd like to remove type incompatibility between definitions, not remove the stream definition.
I was hoping I could figure how to fix this first, but I'm loosing confidence I can as seemingly unrelated things seem to be affected. I've undrafted in hope that others might step in and help. Thanks for shading some light on this. |
I think I just misread one of your messages. Ignore that of mine. |
This reverts commit 13b5a0d.
Could a potential fix be something like this: + import { ReadableStream } from 'node:stream/web'
// Duplicates of interface in lib.dom.ts.
// Duplicated here rather than referencing lib.dom.ts because doing so causes lib.dom.ts to be loaded for "test-all"
// Which in turn causes tests to pass that shouldn't pass.
//
// 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;
+ stream(): ReadableStream;
text(): Promise<string>;
} This would correct the typings, and then hopefully be compatible with the typings in |
Re-ping @microsoft, @DefinitelyTyped, @jkomyno, @alvis, @r3nya, @btoueg, @smac89, @Touffy, @DeividasBakanas, @eyqs, @Hannes-Magnusson-CK, @hoo29, @kjin, @ajafff, @islishude, @mwiktorczyk, @mohsen1, @n-e, @galkin, @parambirs, @eps1lon, @SimonSchick, @ThomasdenH, @WilcoBakker, @wwwy3y3, @samuela, @kuehlein, @bhongy, @chyzwar, @trivikr, @yoursunny, @qwelias, @ExE-Boss, @peterblazejewicz, @addaleax, @victorperin, @ZYSzys, @nodejs, @LinusU, @wafuwafu13: This PR has been out for over a week, yet I haven't seen any reviews. Could someone please give it some attention? Thanks! |
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.
so, from what I understood about the comments: we don't really want to remove stream()
, but instead change the return type somehow, right? @LinusU left a comment suggesting a possible approach: #59057 (comment)
you could try that, and possibly update tests (https://github.com/DefinitelyTyped/DefinitelyTyped/blob/f4c3955fe966c65fe528c1963e0fa217034c6ca5/types/node/test/stream.ts), and see if it fixes your issue and doesn't break anything.
@Gozala One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you! |
@Gozala I haven't seen any activity on this PR in more than three weeks, and it still has problems that prevent it from being merged. The PR will be closed on Apr 14th (in a week) if the issues aren't addressed. |
@Gozala To keep things tidy, we have to close PRs that aren't mergeable and don't have activity in the last month. No worries, though — please open a new PR if you'd like to continue with this change. Thank you! |
@@ -8,7 +8,6 @@ interface Blob { | |||
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.
text(): Promise<string>; | |
stream(): import('node:stream/web').ReadableStream; | |
text(): Promise<string>; |
This somehow never end up in my notifications. I have created new pull request #60377 which does what @LinusU has suggested here I'm not familiar with how things are wired here so I have no idea if this would create some other issues. Either way I think it would be best to remove
If there is a desire to have more correct DefinitelyTyped/types/node/stream/consumers.d.ts Lines 1 to 5 in 063f0c8
Not to mention the fact that only reason this type definition seems to be around is to avoid loading |
I frequently run into following issue
Caused by incorrect type annotation of Blob
DefinitelyTyped/types/node/stream/consumers.d.ts
Line 11 in 2280eb4
Proposed changes replace that annotation with a more accurate one so that it:
lib.dom.d.ts
Blob.prototype.stream
returns web stream and not a node one. See https://nodejs.org/api/buffer.html#blobstreamI do not know how current typedef ended this way. I think comment attempt to elaborate reason for this
Blob
interface definition, yet it does not explain whystream()
method is typed incorrectly.npm test <package to test>
.Select one of these and delete the others:
If adding a new definition:
.d.ts
files generated via--declaration
dts-gen --dt
, not by basing it on an existing project.tslint.json
should contain{ "extends": "@definitelytyped/dtslint/dt.json" }
, and no additional rules.tsconfig.json
should havenoImplicitAny
,noImplicitThis
,strictNullChecks
, andstrictFunctionTypes
set totrue
.If changing an existing definition:
If removing a declaration:
notNeededPackages.json
.