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 body2 #898

Closed
wants to merge 19 commits into from
Closed

feat: return body as consumable body2 #898

wants to merge 19 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jul 23, 2021

This is an alternative variant solving some problems with the previous PR.

  • How to return Node stream?
  • Breaking change.

It's a little hacky but I think it can work out. Thoughts?

@ronag ronag requested a review from mcollina July 23, 2021 06:43
@ronag
Copy link
Member Author

ronag commented Jul 23, 2021

@codecov-commenter
Copy link

codecov-commenter commented Jul 23, 2021

Codecov Report

Merging #898 (50b7d49) into main (02a9d13) will decrease coverage by 1.91%.
The diff coverage is 20.75%.

❗ Current head 50b7d49 differs from pull request most recent head e0e0d84. Consider uploading reports for the commit e0e0d84 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #898      +/-   ##
==========================================
- Coverage   99.57%   97.66%   -1.92%     
==========================================
  Files          26       27       +1     
  Lines        2132     2184      +52     
==========================================
+ Hits         2123     2133      +10     
- Misses          9       51      +42     
Impacted Files Coverage Δ
lib/api/readable.js 19.23% <19.23%> (ø)
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 02a9d13...e0e0d84. Read the comment docs.

@ronag ronag force-pushed the node-stream2 branch 13 times, most recently from 4f1c2ad to 5370b6c Compare July 23, 2021 08:41
@ronag ronag added the enhancement New feature or request label Jul 23, 2021
@ronag ronag force-pushed the node-stream2 branch 4 times, most recently from c43d516 to 344bae6 Compare July 23, 2021 08:52
lib/api/api-request.js Outdated Show resolved Hide resolved
@ronag ronag force-pushed the node-stream2 branch 5 times, most recently from 41dd0a6 to 7068f35 Compare July 23, 2021 10:35
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.

this seems less breaking. Does it wreck our perf?

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

ronag commented Jul 25, 2021

Perf seems same:

main

[bench:run]  Tests                Samples         Result  Tolerance  Difference with slowest 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  http - no keepalive       10   6.34 req/sec   ± 0.65 %                        - 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  http - keepalive          10   7.06 req/sec   ± 1.34 %                + 11.30 % 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  undici - pipeline         10  64.81 req/sec   ± 1.02 %               + 922.39 % 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  undici - stream           10  68.68 req/sec   ± 1.76 %               + 983.36 % 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  undici - dispatch         10  68.86 req/sec   ± 1.53 %               + 986.29 % 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  undici - request          10  70.51 req/sec   ± 2.01 %              + 1012.25 % 

PR

[bench:run]  Tests                Samples         Result  Tolerance  Difference with slowest 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  http - no keepalive       10   6.34 req/sec   ± 0.72 %                        - 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  http - keepalive          10   7.00 req/sec   ± 1.41 %                + 10.48 % 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  undici - pipeline         10  63.50 req/sec   ± 0.90 %               + 902.15 % 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  undici - stream           10  66.57 req/sec   ± 1.58 %               + 950.58 % 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  undici - request          10  70.15 req/sec   ± 2.18 %              + 1007.15 % 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  undici - dispatch         10  72.62 req/sec   ± 1.55 %              + 1046.09 % 

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

ronag commented Jul 25, 2021

I've reduced the ambition here. Remove the hacky performance part and focus on API and functionality to begin with.

@ronag
Copy link
Member Author

ronag commented Jul 25, 2021

Also opened against node core. Waiting for feedback there as well.

@ronag
Copy link
Member Author

ronag commented Jul 28, 2021

I think it's likely we will land a better version in node core. nodejs/node#39520

@ronag ronag closed this Jul 28, 2021
@Uzlopak Uzlopak deleted the node-stream2 branch February 26, 2024 01:41
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.

4 participants