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

feat: return body as consumable body #895

Closed
wants to merge 18 commits into from
Closed

feat: return body as consumable body #895

wants to merge 18 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jul 21, 2021

Change undici.request to return a Body object with
various consume helpers instead of regular node stream.

This aligns with recent work on quic and web streams in
node core.

szmarczak and others added 2 commits July 20, 2021 09:38
* fix: accept URL origin in Agent

* fix: lint

* fixup

Co-authored-by: Robert Nagy <ronagy@icloud.com>
@ronag ronag requested a review from mcollina July 21, 2021 14:15
@ronag
Copy link
Member Author

ronag commented Jul 21, 2021

Tests need to be fixed quite extensively so I ask for some feedback before continuing down this route.

@ronag ronag force-pushed the node-stream branch 3 times, most recently from 9a59a27 to a181898 Compare July 21, 2021 14:18
@ronag
Copy link
Member Author

ronag commented Jul 21, 2021

@Ethan-Arrowood @szmarczak

@ronag ronag requested a review from dnlup July 21, 2021 14:18
@ronag ronag force-pushed the node-stream branch 13 times, most recently from 6913094 to 0347991 Compare July 21, 2021 14:27
@ronag ronag added semver-major Features or fixes that will be included in the next semver major release enhancement New feature or request labels Jul 21, 2021
@ronag ronag force-pushed the node-stream branch 5 times, most recently from 47f70df to c056e71 Compare July 21, 2021 14:41
@mcollina
Copy link
Member

I think this should target a next branch

Copy link
Contributor

@dnlup dnlup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this! I think it's a great improvement in usability.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the concept.

lib/api/api-request.js Outdated Show resolved Hide resolved
@ronag
Copy link
Member Author

ronag commented Jul 22, 2021

Related: nodejs/node#39483. I'm waiting for feedback there and hopefully finalization of the API before we land this so that we can have full compatibility.

@ronag ronag changed the base branch from main to next July 22, 2021 07:59
}

return this[kBody]
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is almost like BodyMixin from the web standard with the small exception of how to consume as Node stream.... anyone got suggestions on how that could/should look? @szmarczak @Ethan-Arrowood @mcollina @dnlup

@ronag ronag force-pushed the node-stream branch 2 times, most recently from 8298466 to 50927b9 Compare July 22, 2021 17:46
mcollina and others added 16 commits July 22, 2021 20:05
* unref() connectTimeout

* clear out the connect timeout in all cases
Change undici.request to return a Body object with
various consume helpers instead of regular node stream.

This aligns with recent work on quic and web streams in
node core.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request semver-major Features or fixes that will be included in the next semver major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants