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

port Header class changes from core PR#38986 #922

Merged
merged 1 commit into from
Aug 4, 2021

Conversation

Ethan-Arrowood
Copy link
Collaborator

This PR ports some of the changes I've made in nodejs/node#38986

Includes an argument length check as well that isn't in my core PR yet

@codecov-commenter
Copy link

codecov-commenter commented Aug 4, 2021

Codecov Report

Merging #922 (083ad52) into main (34cb4c6) will decrease coverage by 1.17%.
The diff coverage is 37.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #922      +/-   ##
==========================================
- Coverage   93.72%   92.55%   -1.18%     
==========================================
  Files          29       29              
  Lines        2517     2564      +47     
==========================================
+ Hits         2359     2373      +14     
- Misses        158      191      +33     
Impacted Files Coverage Δ
lib/core/errors.js 87.50% <0.00%> (-12.50%) ⬇️
lib/api/headers.js 54.12% <42.68%> (-6.69%) ⬇️

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 34cb4c6...083ad52. Read the comment docs.

@ronag ronag merged commit 9ef790e into nodejs:main Aug 4, 2021
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 but it makes me uncomfortable in keep landing changes without tests.

@ronag
Copy link
Member

ronag commented Aug 4, 2021

@mcollina It's only fetch related changes that we land without tests. I think this is good in order to keep velocity up and it adds a natural place for new contributors to help us. If you are still uncomfortable then I guess we can slow down a bit and focus on tests. I was hoping to try and implement functionality and then focus on tests before release.

@ronag
Copy link
Member

ronag commented Aug 4, 2021

@Ethan-Arrowood would be great if we could port over the tests from undici-fetch.

@mcollina
Copy link
Member

mcollina commented Aug 4, 2021

@mcollina It's only fetch related changes that we land without tests. I think this is good in order to keep velocity up and it adds a natural place for new contributors to help us. If you are still uncomfortable then I guess we can slow down a bit and focus on tests. I was hoping to try and implement functionality and then focus on tests before release.

That'd be ok if it's a feature branch and not main. Anyway, let's see what the timeline is... we can always move this into a different branch.

@ronag
Copy link
Member

ronag commented Aug 4, 2021

That'd be ok if it's a feature branch and not main

Yes you are totally right. Apologies.

KhafraDev pushed a commit to KhafraDev/undici that referenced this pull request Jun 23, 2022
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
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.

4 participants