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

Headers API doesn't ignore case #226

Closed
cyco130 opened this issue Nov 2, 2022 · 4 comments · Fixed by #227
Closed

Headers API doesn't ignore case #226

cyco130 opened this issue Nov 2, 2022 · 4 comments · Fixed by #227
Assignees
Labels

Comments

@cyco130
Copy link
Contributor

cyco130 commented Nov 2, 2022

Describe the bug
Headers API should ignore letter cases but it doesn't. It causes many failures in Rakkas's integration tests.

To Reproduce
Run the following snippet:

const headers = new Headers();
headers.append("X-Test", "1234");
console.log("HEADERS", Object.fromEntries(headers.entries()));
console.log(headers.get("X-Test")); // Prints 1234
console.log(headers.get("x-test")); // Prints undefined

Expected behavior
I expect headers.get("x-test") to be equal to headers.get("X-Test").

Information:

  • OS: Linux Mint
  • Browser: Doesn't apply
  • Version: 0.3.3
@cyco130 cyco130 added the bug label Nov 2, 2022
@QuiiBz
Copy link
Member

QuiiBz commented Nov 2, 2022

Good catch! As a note, the implementation is defined here:

  • export class Headers {

    Not sure what's the best solution to fix this bug:
  • Update get/append operations to find by transforming all headers to lowercase
  • Always store the headers in upper/lowercase. That means the user might be surprised because we modify a header key on his behalf (it shouldn't change anything at the end, because any get operation will return the right header value no matter the case of the header key)

@cyco130
Copy link
Contributor Author

cyco130 commented Nov 2, 2022

Sounds like a perfect first PR opportunity :)

I haven't looked at the spec but Chrome, Firefox, Deno, and Node (with --experimental-fetch) all return "x-test" for [...new Headers({ "X-Test" : "1234" })][0][0]. The end user has very few ways to observe the behavior so I think storing everything in lowercase should be OK.

Would you accept a getSetCookie or getAll("set-cookie") implementation in the same PR or should we open a discussion issue for that?

@cyco130
Copy link
Contributor Author

cyco130 commented Nov 2, 2022

The spec also seems to approve of storing in lowercase.

@QuiiBz
Copy link
Member

QuiiBz commented Nov 2, 2022

The spec also seems to approve of storing in lowercase.

Sounds great. Let's go this way, and let me know if you want to submit a PR for this :)

Would you accept a whatwg/fetch#1346 implementation in the same PR or should we open a discussion issue for that?

I would prefer opening a new issue, since it's not directly related to this bug.

QuiiBz added a commit that referenced this issue Nov 2, 2022
* fix(js-runtime): make Headers class case insensitive (#226)

* fix: runtime test

Co-authored-by: QuiiBz <tom.lienrd@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants