fix: errored stream is disturbed #1134

ronag · 2021-12-08T19:51:48Z

No description provided.

test/node-fetch/main.js

ronag · 2021-12-08T19:53:53Z

@szmarczak This is a spec thing. Care to help clarifying what is the correct behaviour.

codecov-commenter · 2021-12-08T19:59:23Z

Codecov Report

Merging #1134 (2cc7067) into main (b905cea) will decrease coverage by 0.00%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##             main    #1134      +/-   ##
==========================================
- Coverage   94.30%   94.30%   -0.01%     
==========================================
  Files          40       40              
  Lines        3809     3808       -1     
==========================================
- Hits         3592     3591       -1     
  Misses        217      217

Impacted Files	Coverage Δ
lib/fetch/symbols.js	`100.00% <ø> (ø)`
lib/fetch/index.js	`82.28% <75.00%> (ø)`
lib/core/util.js	`98.44% <100.00%> (ø)`
lib/fetch/body.js	`99.16% <100.00%> (-0.01%)`	⬇️

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 b905cea...2cc7067. Read the comment docs.

ronag · 2021-12-08T20:03:51Z

Chrome seems to go with TypeError.

ronag · 2021-12-08T20:04:48Z

@jimmywarting: Any thoughts? I believe node-fetch fails with an AbortError in this test.

jimmywarting · 2021-12-08T20:13:16Z

Umm. is there any wpt test or something in the spec that mention what should actually be happening?

lib/fetch/index.js

Refs: nodejs/undici#1134

szmarczak · 2021-12-09T14:32:59Z

Sorry for my inactivity, had a few tests and an exam in uni. Will take a look now.

lib/core/util.js

test/node-fetch/main.js

szmarczak · 2021-12-09T15:03:07Z

@jimmywarting whatwg/streams#1025 tl;dr unfortunately it's up to Node.js, because they made a design mistake to allow fetch access stream internals.

mcollina

Can you please add a unit test?

ronag · 2021-12-09T20:38:20Z

Can you please add a unit test?

There is already unit test? I updated the relevant one.

mcollina · 2021-12-10T07:11:15Z

I missed it. CI is failing.

mcollina

lgtm

szmarczak

Indeed, it should be TypeError 👍

Refs: nodejs/undici#1134 PR-URL: #41121 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

Refs: nodejs/undici#1134 PR-URL: nodejs#41121 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

Refs: nodejs/undici#1134 PR-URL: #41121 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

* fix: errored stream is disturbed * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup

fix: errored stream is disturbed

1a8313c

ronag commented Dec 8, 2021

View reviewed changes

test/node-fetch/main.js Show resolved Hide resolved

ronag added 3 commits December 8, 2021 20:58

fixup

1671eda

fixup

8e317f4

fixup

08cc40b

fixup

656f8f9

ronag added 2 commits December 8, 2021 21:09

fixup

07d45d5

fixup

f8f14b6

ronag requested a review from mcollina December 8, 2021 20:10

mcollina reviewed Dec 8, 2021

View reviewed changes

lib/fetch/index.js Show resolved Hide resolved

ronag added a commit to nxtedition/node that referenced this pull request Dec 9, 2021

stream: add isErrored helper

eeab6d4

Refs: nodejs/undici#1134

ronag mentioned this pull request Dec 9, 2021

stream: add isErrored helper nodejs/node#41121

Closed

ronag added a commit to nxtedition/node that referenced this pull request Dec 9, 2021

stream: add isErrored helper

46092fb

Refs: nodejs/undici#1134

ronag added a commit to nxtedition/node that referenced this pull request Dec 9, 2021

stream: add isErrored helper

9c79a04

Refs: nodejs/undici#1134

ronag added a commit to nxtedition/node that referenced this pull request Dec 9, 2021

stream: add isErrored helper

c76beb4

Refs: nodejs/undici#1134

ronag added a commit to nxtedition/node that referenced this pull request Dec 9, 2021

stream: add isErrored helper

1e2413b

Refs: nodejs/undici#1134

fixup

00decc2

ronag requested a review from mcollina December 9, 2021 09:40

ronag added 3 commits December 9, 2021 10:40

fixup

cc98805

fixup

fd16eba

fixup

0f694eb

ronag added a commit to nxtedition/node that referenced this pull request Dec 9, 2021

stream: add isErrored helper

7ade6e3

Refs: nodejs/undici#1134

ronag added 2 commits December 9, 2021 15:38

fixup

cffd81b

fixup

26e8031

ronag force-pushed the body-error branch from 2529848 to 26e8031 Compare December 9, 2021 14:39

szmarczak reviewed Dec 9, 2021

View reviewed changes

lib/core/util.js Outdated Show resolved Hide resolved

test/node-fetch/main.js Show resolved Hide resolved

mcollina reviewed Dec 9, 2021

View reviewed changes

mcollina approved these changes Dec 10, 2021

View reviewed changes

ronag added 6 commits December 10, 2021 08:53

fixup

db66684

fixup

6e1d7a0

fixup

017bd46

fixup

3160a64

fixup

4f47b61

fixup

2cc7067

szmarczak approved these changes Dec 10, 2021

View reviewed changes

ronag merged commit a3c0413 into main Dec 10, 2021

ronag added a commit to nodejs/node that referenced this pull request Dec 16, 2021

stream: add isErrored helper

752d75d

Refs: nodejs/undici#1134 PR-URL: #41121 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

danielleadams pushed a commit to nodejs/node that referenced this pull request Dec 16, 2021

stream: add isErrored helper

9b0cafd

Refs: nodejs/undici#1134 PR-URL: #41121 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

danielleadams pushed a commit to nodejs/node that referenced this pull request Dec 17, 2021

stream: add isErrored helper

b59c513

Refs: nodejs/undici#1134 PR-URL: #41121 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

danielleadams pushed a commit to nodejs/node that referenced this pull request Jan 31, 2022

stream: add isErrored helper

2568ac9

Refs: nodejs/undici#1134 PR-URL: #41121 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022

stream: add isErrored helper

b7c0d6c

Refs: nodejs/undici#1134 PR-URL: nodejs#41121 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

danielleadams pushed a commit to nodejs/node that referenced this pull request Feb 1, 2022

stream: add isErrored helper

4522216

Refs: nodejs/undici#1134 PR-URL: #41121 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

Uzlopak deleted the body-error branch February 21, 2024 12:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: errored stream is disturbed #1134

fix: errored stream is disturbed #1134

ronag commented Dec 8, 2021

ronag commented Dec 8, 2021

codecov-commenter commented Dec 8, 2021 •

edited

Loading

ronag commented Dec 8, 2021 •

edited

Loading

ronag commented Dec 8, 2021

jimmywarting commented Dec 8, 2021

szmarczak commented Dec 9, 2021

szmarczak commented Dec 9, 2021 •

edited

Loading

mcollina left a comment

ronag commented Dec 9, 2021 •

edited

Loading

mcollina commented Dec 10, 2021

mcollina left a comment

szmarczak left a comment •

edited

Loading

fix: errored stream is disturbed #1134

fix: errored stream is disturbed #1134

Conversation

ronag commented Dec 8, 2021

ronag commented Dec 8, 2021

codecov-commenter commented Dec 8, 2021 • edited Loading

Codecov Report

ronag commented Dec 8, 2021 • edited Loading

ronag commented Dec 8, 2021

jimmywarting commented Dec 8, 2021

szmarczak commented Dec 9, 2021

szmarczak commented Dec 9, 2021 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Dec 9, 2021 • edited Loading

mcollina commented Dec 10, 2021

mcollina left a comment

Choose a reason for hiding this comment

szmarczak left a comment • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Dec 8, 2021 •

edited

Loading

ronag commented Dec 8, 2021 •

edited

Loading

szmarczak commented Dec 9, 2021 •

edited

Loading

ronag commented Dec 9, 2021 •

edited

Loading

szmarczak left a comment •

edited

Loading