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: implement body mixin on Readable #907

Merged
merged 14 commits into from
Jul 31, 2021
Merged

feat: implement body mixin on Readable #907

merged 14 commits into from
Jul 31, 2021

Conversation

ronag
Copy link
Member

@ronag ronag commented Jul 30, 2021

Port over PR from node core. It will not be possible to land
these changes on core due to ecosystem breakage.

Refs: nodejs/node#39520

Port over PR from node core. It will not be possible to land
these changes on core due to ecosystem breakage.

Refs: nodejs/node#39520
@ronag ronag requested a review from mcollina July 30, 2021 08:26
@codecov-commenter
Copy link

codecov-commenter commented Jul 30, 2021

Codecov Report

Merging #907 (b19c11b) into main (11c2db1) will decrease coverage by 1.05%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #907      +/-   ##
==========================================
- Coverage   99.57%   98.52%   -1.06%     
==========================================
  Files          26       27       +1     
  Lines        2132     2230      +98     
==========================================
+ Hits         2123     2197      +74     
- Misses          9       33      +24     
Impacted Files Coverage Δ
lib/api/readable.js 77.35% <77.35%> (ø)
lib/api/api-request.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11c2db1...b19c11b. Read the comment docs.

@ronag ronag force-pushed the readable.body branch 7 times, most recently from 2805b1f to 7334d8c Compare July 30, 2021 08:47
@ronag ronag force-pushed the readable.body branch 11 times, most recently from 37557e3 to 5a3ab2e Compare July 30, 2021 09:20
@ronag ronag force-pushed the readable.body branch 4 times, most recently from 8d744a2 to d5e1f98 Compare July 30, 2021 10:04
@ronag ronag changed the title stream: implement body mixin on Readable feat: implement body mixin on Readable Jul 30, 2021
@ronag ronag added the enhancement New feature or request label Jul 30, 2021
lib/api/readable.js Show resolved Hide resolved
lib/api/readable.js Show resolved Hide resolved
Copy link
Collaborator

@Ethan-Arrowood Ethan-Arrowood left a comment

Choose a reason for hiding this comment

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

LGTM - do you want to make the type updates as well? Shouldn't be hard for this change

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.

Code looks good. I would recommend we mention what methods will be available in the docs. The .json() method might also deserve a special mention in the README

@ronag
Copy link
Member Author

ronag commented Jul 31, 2021

Fixed

@ronag ronag requested a review from mcollina July 31, 2021 10:57
@ronag
Copy link
Member Author

ronag commented Jul 31, 2021

master

[bench:run] │ Tests               │ Samples │        Result │ Tolerance │ Difference with slowest │
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run] │ http - no keepalive │      10 │  6.39 req/sec │  ± 0.76 % │                       - │
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run] │ http - keepalive    │      10 │  7.14 req/sec │  ± 1.22 % │               + 11.87 % │
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run] │ undici - pipeline   │      10 │ 67.28 req/sec │  ± 1.88 % │              + 953.57 % │
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run] │ undici - request    │      10 │ 68.88 req/sec │  ± 1.87 % │              + 978.72 % │
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run] │ undici - stream     │      10 │ 70.55 req/sec │  ± 1.32 % │             + 1004.83 % │
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run] │ undici - dispatch   │      10 │ 75.16 req/sec │  ± 2.16 % │             + 1076.97 % │

PR

[bench:run]  Tests                Samples         Result  Tolerance  Difference with slowest 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  http - no keepalive       10   6.43 req/sec   ± 0.98 %                        - 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  http - keepalive          10   6.85 req/sec   ± 1.93 %                 + 6.42 % 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  undici - pipeline         10  63.27 req/sec   ± 2.41 %               + 883.47 % 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  undici - stream           10  67.86 req/sec   ± 1.31 %               + 954.73 % 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  undici - request          10  68.78 req/sec   ± 2.16 %               + 969.07 % 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  undici - dispatch         10  73.52 req/sec   ± 1.72 %              + 1042.76 % 

@ronag
Copy link
Member Author

ronag commented Jul 31, 2021

@mcollina doesn't seem to have any significant perf regression.

@ronag ronag requested a review from mcollina July 31, 2021 14:14
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.

lgtm

@ronag ronag merged commit c0e91b8 into main Jul 31, 2021
ronag added a commit to nodejs/node that referenced this pull request Aug 2, 2021
This reverts commit 8306051.

PR-URL: #39589
Refs: nodejs/undici#907
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Trott pushed a commit to nodejs/node that referenced this pull request Aug 2, 2021
Adds did read accessor used to determine whether a readable has been
read from.

PR-URL: #39589
Refs: nodejs/undici#907
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
danielleadams pushed a commit to nodejs/node that referenced this pull request Aug 16, 2021
Adds did read accessor used to determine whether a readable has been
read from.

PR-URL: #39589
Refs: nodejs/undici#907
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
targos pushed a commit to nodejs/node that referenced this pull request Sep 4, 2021
Adds did read accessor used to determine whether a readable has been
read from.

PR-URL: #39589
Refs: nodejs/undici#907
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
KhafraDev pushed a commit to KhafraDev/undici that referenced this pull request Jun 23, 2022
* stream: implement body mixin on Readable

Port over PR from node core. It will not be possible to land
these changes on core due to ecosystem breakage.

Refs: nodejs/node#39520

* fixup: tests

* fixup

* fixup

* fixuP

* fixuP

* fixup: remove node specific stuff

* fixup: formData

* fixup: simplify

* fixup

* fixup

* fixup

* fixup: README

* fixup
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
* stream: implement body mixin on Readable

Port over PR from node core. It will not be possible to land
these changes on core due to ecosystem breakage.

Refs: nodejs/node#39520

* fixup: tests

* fixup

* fixup

* fixuP

* fixuP

* fixup: remove node specific stuff

* fixup: formData

* fixup: simplify

* fixup

* fixup

* fixup

* fixup: README

* fixup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants