stream: implement fetch body mixin on Readable

[ronag]

Make Readable expose the fetch boxy mixin API.

Bypasses webstream glue when possible.

This also makes conversion to webstream faster when the stream is not consumed through Readable by bypassing much of the Readable logic.

Refs: https://fetch.spec.whatwg.org/#body-mixin

[ronag]

Not sure if adding properties on core classes should be semver-major?

[ronag]

Will work on test if there is positive feedback on this approach.

[ronag]

@nodejs/streams

[ronag]

Readable.body seems to be used in eco system. See CITGM (https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/nodes=rhel7-s390x/2735/testReport/).

Any ideas on how to get around this?

1. Don't implement body mixin fully, i.e. skip body.
2. Implement some kind of compat where if body has been set the behaviour is overriden?
3. Other ideas?

@lpinca @jasnell @mcollina @dnlup

https://github.com/visionmedia/superagent/blob/master/src/node/response.js#L36
https://github.com/expressjs/body-parser/blob/master/lib/types/json.js#L105
https://github.com/expressjs/multer/blob/master/lib/make-middleware.js#L28
https://github.com/expressjs/multer/blob/master/lib/make-middleware.js#L92

@dougwilson @niftylettuce

[ronag]

The express stuff can be workaround through, but the superagent issue is maybe more difficult... https://github.com/visionmedia/superagent/blob/master/src/node/response.js#L36

[mcollina]

I don't think this is doable without massive ecosystem breakage. We need also to consider Duplex inherit from Readable, and therefore it will inherit a .json() method. However express provides a res.json() method, which will quickly get confused with this.

I think we are better off with a separate class, or some other utility Readable.toFetchBody(readable).json() or readable.toFetchBody().json().

[ronag]

I don't think this is doable without massive ecosystem breakage. We need also to consider Duplex inherit from Readable, and therefore it will inherit a .json() method. However express provides a res.json() method, which will quickly get confused with this.

I think we are better off with a separate class, or some other utility Readable.toFetchBody(readable).json() or readable.toFetchBody().json().

That kind of kills the use case for me. Can we put it behind a flag passed to the constructor which is disabled by default?

[mcollina]

That kind of kills the use case for me. Can we put it behind a flag passed to the constructor which is disabled by default?

This means that most Node.js core object would not have those. I'm not sure that's what you would like achieve either.
What's your use case specifically?

[ronag]

That kind of kills the use case for me. Can we put it behind a flag passed to the constructor which is disabled by default?

This means that most Node.js core object would not have those. I'm not sure that's what you would like achieve either.
What's your use case specifically?

We wanted to support body mixin in undici request API without causing breaking change. Also it makes sense there in general.

[ronag]

Taking another look at this I believe this can actually live outside of core (at least for my intended use case). nodejs/undici#907

[ronag]

Given the issue described in #39543, can we at least keep bodyUsed (under some other name if necessary)? Also possibly this[kReading] under some public API (maybe can be merged into readableDidRead)?

[ronag]

I'm closing this. It's not possible to do it this way due ecosystem conflicts. If someone wants to pick it up in some other form feel free to branch off this or create an issue for discussion. I'm moving over to extending Readable in user land where needed. nodejs/undici#907

[RomainLanz]

If I may give my feedback on this.

I am sad that this is closed partially due to an outdated framework/library (express) that is already not working with modern JavaScript (AsyncFunction are causing memory leaks and crashes).

There are no reason not to add this feature inside the core, especially if it is part of a major release (meaning their will be breaking changes).

Libraries and frameworks need to adapt. It is not the role of Node.js to be adjusted.

Everyone knows that patching/modifying core/native objects is a very bad practice. We should not rely on people who are doing that to keep us from improving Node.js core.

[benjamingr]

Libraries and frameworks need to adapt. It is not the role of Node.js to be adjusted.

I see (and appreciate) the sentiment but that's not the sort of burden of responsibility Node.js has. Express is used by millions of projects and we can't just break it.

@ronag

What about a mixinBody or similar method that enriches Readable (or a constructor option that opts into this)?

[ronag]

What about a mixinBody or similar method that enriches Readable (or a constructor option that opts into this)?

I'm good with either.

[ronag]

@benjamingr something like 1b7f509?

[benjamingr]

@ronag exactly :)

[benjamingr]

Then different kinds of streams can opt-into this by default (so http IncomingRequest stream won't be able to by default but http.get might etc)

[mcollina]

Then different kinds of streams can opt-into this by default (so http IncomingRequest stream won't be able to by default but http.get might etc)

This will make the API even harder to learn/use for folks.

[benjamingr]

What do you suggest instead?

[jakearchibald]

Could there just be a module of stream consumers?

import { asJson } from 'stream-readers';

const data = await asJson(stream);

I might be missing some context around this PR.

[Mesteery]

Could there just be a module of stream consumers?

#39594