-
Notifications
You must be signed in to change notification settings - Fork 41
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 network.BytesValue
type
#472
Conversation
@juliandescottes can you review this? |
This provides a uniform representation of network data that may be UTF-8 text or may be arbitary binary data. Non-UTF-8 data is always encoded as base64 for transport. This is a backwards-incompatible change since it coverts the network.Cookie and network.Headers types from either having a `value` or `binaryValue` field to always having a `value` field, but the value being an object with a `type` field to distinguish the variants.
efa3987
to
664d2df
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, some questions about the deserialize/serialize steps, but either I'm misunderstanding it, or it should be easy to address.
index.bs
Outdated
1. If |bytes| matches the <code>network.StringValue</code> production, | ||
let |protocol value| be [=UTF-8 encode=] |bytes|["<code>value</code>"]. | ||
|
||
1. Otherwise if |bytes| matches the <code>network.Base64Value</code> | ||
production. Let |protocol value| be [=forgiving-base64 decode=] | ||
|bytes|["<code>value</code>"]. | ||
|
||
1. Return |protocol value|. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the serialization steps? We should not expect bytes
to match one of those productions, but rather decide based on whether it's valid UTF 8 or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow, yes, this is total nonsense :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now should be fixed.
Co-authored-by: Julian Descottes <jdescottes@mozilla.com> Co-authored-by: Thiago Perrotta <tperrotta@chromium.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm after the last update.
This provides a uniform representation of network data that may be UTF-8 text or may be arbitary binary data. Non-UTF-8 data is always encoded as base64 for transport.
This is a backwards-incompatible change since it coverts the network.Cookie and network.Headers types from either having a
value
orbinaryValue
field to always having avalue
field, but the value being an object with atype
field to distinguish the variants.Preview | Diff