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 options to stream consumers #40111

Closed
jimmywarting opened this issue Sep 14, 2021 · 6 comments
Closed

add options to stream consumers #40111

jimmywarting opened this issue Sep 14, 2021 · 6 comments
Labels
feature request Issues that request new features to be added to Node.js. stale stream Issues and PRs related to the stream subsystem.

Comments

@jimmywarting
Copy link

@jimmywarting:

i experimented with stream consumer a bit... i think it could be useful if blob accepted a optional mime type as well (just a suggestion)

Yeah, let's take that to a separate issue. I think adding an options argument to each of the convenience consumers makes sense.

Originally posted by @jasnell in #40089 (comment)

@jimmywarting
Copy link
Author

jimmywarting commented Sep 14, 2021

it would also be useful if arrayBuffer(iterable, [totalSize]) accepted a total size so you could pre allocate the ArrayBuffer and put in each chunk at the right location instead of using 2x memory

...don't have any other suggestions for json or text
always use TextDecoder, utf8 and strip BOM like it already dose

@Mesteery Mesteery added stream Issues and PRs related to the stream subsystem. feature request Issues that request new features to be added to Node.js. labels Sep 15, 2021
@ronag
Copy link
Member

ronag commented Sep 15, 2021

I think these are thought to mirror the body mixin from the fetch standard.

@jimmywarting
Copy link
Author

jimmywarting commented Sep 15, 2021

Yea, but what response.blob() dose is that it reads the content-type header and assign it to the type

if you want to turn any iterable into a blob then you have no other way to set the correct type other than creating 2 blobs.

chunk = new Blob([await blob(iterable)], {type: 'text/html'})

i where looking into how we could use this user-land version of stream-consumers in node-fetch
but there are some improvements we would like to have in order to optimise the usage.


This arrayBuffer() is also very useful for other areas as well, such as concatinating the streams into a single large arraybuffer... If it has knowledge of total size (content-length) then it can optimise the allocation for large data in adv.

these 2 missing optimisation makes it a tiny bit unappealing for us to use stream-consumer at all.


I also wished that buffer(iterable) wasn't added... it's no good for other places other than in NodeJS. i just think it pollutes the Deno/browser land and runs slower... A await uint8array(iterable) is something that i could stand behind...

i mean buffer() is not part of the body mixin standard...

@clshortfuse
Copy link
Contributor

clshortfuse commented Mar 31, 2022

I'm rewriting my framework to use fetch functions. Here's what it looks like now, without the ability to set the content-type in blob via stream/consumers:

/** @type {import('stream/consumers')} */
let streamConsumers;
try {
  streamConsumers = await import(new URL('node:stream/consumers').toString());
} catch {}

let BlobClass = (typeof Blob === 'undefined' ? undefined : Blob);

export default class HttpRequest {

  /** snip **/

  async blob() {
    if (streamConsumers) {
      const contentType = this.headers['content-type'];
      // SEE HERE: Adding the option in case it's added in the future.
      const result = await streamConsumers.blob(this.stream, {
        type: contentType,
      });
      this.#bodyUsed = true;
      if (!contentType || result.type === contentType) {
        return result;
      }
      // SEE HERE: Proxy needed to set type.
      return new Proxy(result, {
        get: (target, p, receiver) => {
          if (p === 'type') return this.headers['content-type'];
          return Reflect.get(target, p, receiver);
        },
      });
    }

    if (BlobClass === undefined) {
      try {
        const module = await import('node:buffer');
        if ('Blob' in module === false) throw new Error('NOT_SUPPORTED');
        BlobClass = module.Blob;
      } catch {
        BlobClass = null;
      }
    }

    if (BlobClass === null) {
      throw new Error('NOT_SUPPORTED');
    }

    try {
      const chunks = [];
      for await (const chunk of this.stream) { chunks.push(chunk); }
      return new BlobClass(chunks, {
        type: this.headers['content-type'],
      });
    } finally {
      this.#bodyUsed = true;
    }
  }

I'm not entirely clear on spec, but I think if somebody calls one function, like .json(), you can't then call .blob(). It will have already been consumed. So, we (NodeJS) shouldn't worry about somehow tagging the mime type on the stream. It would be the work of the user (or in this case me as the framework designer) to build a to-spec implementation of the body mixins. I reason stream/consumers don't have to be built to spec, but should allow for a framework to build a to-spec implementation with it. Therefore, stream/consumers should allow extra options outside of the fetch body spec.

@github-actions
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Sep 28, 2022
@github-actions
Copy link
Contributor

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. stale stream Issues and PRs related to the stream subsystem.
Projects
Development

No branches or pull requests

4 participants