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

fix: readable.setEncoding #1129

Closed
wants to merge 5 commits into from
Closed

fix: readable.setEncoding #1129

wants to merge 5 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Dec 7, 2021

@ronag
Copy link
Member Author

ronag commented Dec 7, 2021

@koh110 Would you mind fixing tests?

@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2021

Codecov Report

Merging #1129 (a0a0f72) into main (ebea0f7) will increase coverage by 2.14%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1129      +/-   ##
==========================================
+ Coverage   94.25%   96.40%   +2.14%     
==========================================
  Files          40       30      -10     
  Lines        3815     2501    -1314     
==========================================
- Hits         3596     2411    -1185     
+ Misses        219       90     -129     
Impacted Files Coverage Δ
lib/api/readable.js 79.31% <83.33%> (-9.49%) ⬇️
index.js 85.13% <0.00%> (-14.87%) ⬇️
lib/core/util.js 89.14% <0.00%> (-9.31%) ⬇️
lib/handler/redirect.js 87.34% <0.00%> (-7.60%) ⬇️
lib/core/errors.js 91.39% <0.00%> (-3.23%) ⬇️
lib/fetch/formdata.js
lib/fetch/constants.js
lib/fetch/body.js
lib/fetch/util.js
... and 6 more

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 ebea0f7...a0a0f72. Read the comment docs.

@koh110
Copy link
Contributor

koh110 commented Dec 7, 2021

@ronag Thanks for your PR. Here is my test code.
Would you review it?
#1131

@ronag ronag requested a review from mcollina December 7, 2021 15:04
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
Copy link
Member Author

ronag commented Dec 7, 2021

Fixing this is very complicated unfortunately... :/

@ronag ronag marked this pull request as draft December 7, 2021 16:17
@koh110 koh110 mentioned this pull request Dec 8, 2021
@koh110
Copy link
Contributor

koh110 commented Dec 8, 2021

@ronag @mcollina Hi. I tried to fix this bug.
But I'm not sure if I've fixed it perfectly.
Would you review it?
#1133

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

client.request throws Error with setEncoding
4 participants