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(headers): iterate over cloned list #1081

Merged
merged 3 commits into from
Nov 1, 2021

Conversation

lukeed
Copy link
Contributor

@lukeed lukeed commented Oct 31, 2021

This matches the browser behavior, example:

let demo = new Headers({ foo: '123', bar: '456' });

for (let [k,v] of demo) { 
  demo.delete(k);
  demo.set(`x-${k}`, v);
}

[...demo];
//=> [
//=>   ['x-bar', '456'],
//=>   ['x-foo', '123'],
//=> ]

Note: This is included as a test

@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2021

Codecov Report

Merging #1081 (a6c8b23) into main (2fadc07) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1081   +/-   ##
=======================================
  Coverage   95.09%   95.09%           
=======================================
  Files          39       39           
  Lines        3750     3753    +3     
=======================================
+ Hits         3566     3569    +3     
  Misses        184      184           
Impacted Files Coverage Δ
lib/fetch/headers.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 2fadc07...a6c8b23. Read the comment docs.

@ronag
Copy link
Member

ronag commented Oct 31, 2021

If you could also find ref to spec that would be great.

@lukeed
Copy link
Contributor Author

lukeed commented Oct 31, 2021

From the last sentence in this section

The value pairs to iterate over are the return value of running sort and combine with this’s header list.

Where sort and combine is an instruction-set for creating a new list.

@lukeed
Copy link
Contributor Author

lukeed commented Oct 31, 2021

Adding another test to this PR, which is also failing in main branch

@ronag ronag requested a review from Ethan-Arrowood October 31, 2021 19:37
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

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!

@Ethan-Arrowood
Copy link
Collaborator

The infinite iteration test is a good check as well. Totally missed that; I wonder if something like that would be valuable for the web platform tests

@ronag ronag merged commit 39aca4a into nodejs:main Nov 1, 2021
@lukeed lukeed deleted the fix/headers-iterator branch November 1, 2021 16:59
KhafraDev pushed a commit to KhafraDev/undici that referenced this pull request Jun 23, 2022
* chore(headers): add failing test

* fix(headers): iterate over list clone

* chore(headers): add test for infinite iteration
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
* chore(headers): add failing test

* fix(headers): iterate over list clone

* chore(headers): add test for infinite iteration
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.

5 participants