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

Add fields.has #91

Merged
merged 5 commits into from
Dec 4, 2023
Merged

Add fields.has #91

merged 5 commits into from
Dec 4, 2023

Conversation

elliottt
Copy link
Collaborator

@elliottt elliottt commented Dec 4, 2023

It's not currently possible to test whether a field-key is present in a fields, as our return value of an empty list could mean either that the header was missing, or had no value. Add the fields.has method to give an unambiguous way to test for field-key presence in a fields.

Fixes #82

@pchickey
Copy link
Contributor

pchickey commented Dec 4, 2023

This comment is now obsolete

Before we land this I believe we need to make this change in structure consistent in the other field methods:

  • from-list can wrap field-value in option
  • set can wrap list<field-value> in option
  • entries can wrap field-value in option.

@elliottt
Copy link
Collaborator Author

elliottt commented Dec 4, 2023

After some discussion, I think the right change here is instead to add the fields.has function that @guybedford mentioned in #82. This addresses the immediate concern of being able to test if a header is present in the fields, and requires no other spec changes.

@elliottt elliottt force-pushed the trevor/fallible-get branch from e45c02d to 2f3ac89 Compare December 4, 2023 23:16
@elliottt elliottt changed the title Change fields.get to return an option Add fields.has Dec 4, 2023
@guybedford
Copy link
Contributor

Could we implement both changes?

Copy link
Contributor

@pchickey pchickey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to adding fields.has. I appreciate that we are able to avoid changing the types on any other fields methods by using "" to represent the empty value.

@elliottt elliottt force-pushed the trevor/fallible-get branch from 2f3ac89 to 7e2ea3c Compare December 4, 2023 23:48
@elliottt
Copy link
Collaborator Author

elliottt commented Dec 4, 2023

Could we implement both changes?

I think that since we're using "" to mean present-but-empty, this would mean that a return value of [] indicates that the key is missing. I've updated the docs on fields.get to indicate that [""] is present-but-empty, and [] is missing.

elliottt and others added 2 commits December 4, 2023 15:52
Co-authored-by: Pat Hickey <pat@moreproductive.org>
@elliottt elliottt force-pushed the trevor/fallible-get branch from 461e3c6 to 32dd769 Compare December 4, 2023 23:52
@elliottt elliottt merged commit 21bdfda into WebAssembly:main Dec 4, 2023
@guybedford
Copy link
Contributor

guybedford commented Dec 5, 2023

This sounds reasonable in equating the empty string with an empty header value. This then makes it easy to set an empty header and compose it the other functions like append and set as @pchickey mentions in #91 (comment). In the WhatWG implementation var h = new Headers({ a: '' }); h.append('A', ''); h.get('a'); returns " , ", which seems highly unnecessary so I do prefer this approach for consistency.

Edit: I think this approach still allows multiple empty header values just fine in that append('a', '') can be called twice, so it is whatwg consistent even and more so than with a get option<>.

@elliottt elliottt deleted the trevor/fallible-get branch December 5, 2023 19:04
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.

fields.get returning an option for not found
4 participants