fix: Blob.stream return type which is incorrect and conflicts with lib.dom.d.ts

[Gozala]

I frequently run into following issue

Caused by incorrect type annotation of Blob

DefinitelyTyped/types/node/stream/consumers.d.ts

Line 11 in 2280eb4

stream(): NodeJS.ReadableStream;

Proposed changes replace that annotation with a more accurate one so that it:

1. Does not conflict with lib.dom.d.ts
2. Reflects actual state of things in node & specifically the fact that Blob.prototype.stream returns web stream and not a node one. See https://nodejs.org/api/buffer.html#blobstream

I 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 why stream() method is typed incorrectly.

• Use a meaningful title for the pull request. Include the name of the package modified.
• Test the change in your own code. (Compile and run.)
• Add or edit tests to reflect the change.
• Follow the advice from the readme.
• Avoid common mistakes.
• Run npm test <package to test>.

Select one of these and delete the others:

If adding a new definition:

• The package does not already provide its own types, or cannot have its .d.ts files generated via --declaration
• If this is for an npm package, match the name. If not, do not conflict with the name of an npm package.
• Create it with dts-gen --dt, not by basing it on an existing project.
• Represents shape of module/library correctly
• tslint.json should contain { "extends": "@definitelytyped/dtslint/dt.json" }, and no additional rules.
• tsconfig.json should have noImplicitAny, noImplicitThis, strictNullChecks, and strictFunctionTypes set to true.

If changing an existing definition:

• Provide a URL to documentation or source code which provides context for the suggested changes: <>
• 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.

If removing a declaration:

• If a package was never on Definitely Typed, you don't need to do anything. (If you wrote a package and provided types, you don't need to register it with us.)
• Delete the package's directory.
• Add it to notNeededPackages.json.

[Gozala]

Looks like issue was raised during review #55311 (comment) but things have landed anyhow.

[Gozala]

@favna I think you've introduced this definition into codebase, mind providing some context here and whether proposed changes would cause problems ?

Thanks

[Gozala]

Looks like ReadableStream is defined in lib.dom.d.ts which as per comment should not be referenced otherwise failing tests would pass as well.

Again I'm not sure I understand the intricacies here, only other alternative I can think of is removing stream method all together so it does not conflict with lib.dom.d.ts yet it feels like proper fix here would be to reference actual Blob instead.

[Gozala]

Looks like there is another place in node where Blob is defined, although there stream returns unknown 🤷‍♂️

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 buffer module. That one may not be ideal, but at least it will not cause conflicts and hopefully would get converged with dom.lib.d.ts in the future.

[favna]

@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.

[favna]

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.

[typescript-bot]

@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 PR

• node — on npm, on unpkg

Code Reviews

Because 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

• ✅ No merge conflicts
• ✅ Continuous integration tests have passed
• 🕐 Only a DT maintainer can approve changes without tests

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.

Inactive

This 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"
}

[typescript-bot]

🔔 @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 Approve or Request Changes in the GitHub UI so I know what's going on.

[typescript-bot]

@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.

[Gozala]

@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.

@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.

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 not sure I fully understand the relation here. I'd like to remove type incompatibility between definitions, not remove the stream definition.

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.

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.

[favna]

Obviously we cannot fully remove stream because it is actually functionality that's in NodeJS: nodejs.org/dist/latest-v17.x/docs/api/stream.html

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 think I just misread one of your messages. Ignore that of mine.

[LinusU]

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 dom.d.ts?

[typescript-bot]

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!

[gabritto]

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.

[typescript-bot]

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

[typescript-bot]

@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.

[typescript-bot]

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

[Gozala]

Suggested change

	text(): Promise<string>;
	stream(): import('node:stream/web').ReadableStream;
	text(): Promise<string>;

[Gozala]

This somehow never end up in my notifications. I have created new pull request #60377 which does what @LinusU has suggested here
#59057 (comment)

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 stream() method as:

1. It is simply incorrect
2. Causes problems caused by conflicts with built in typedefs.
3. No tests seem to break removing it presumably because nothing actually depends on it.

If there is a desire to have more correct stream() method that could be pursued separately as an improvement. First change would still resolve incompatibility issues without blocking on figuring out all the quirks of how outlined here

DefinitelyTyped/types/node/stream/consumers.d.ts

Lines 1 to 5 in 063f0c8

	// 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.

Not to mention the fact that only reason this type definition seems to be around is to avoid loading Blob definition form dom.d.ts which I'm still not sure I understand why is that desired.