Change ProtoByteString to use its own class rather than String | Uint8Array
#2639
Conversation
thebrianchen
changed the title
Blob rather than String | Uint8ArrayProtoByteString to use Blob rather than String | Uint8Array
|
Took a first pass. @schmidt-sebastian See if you disagree with any of the feedback here. On Android we have a distinction between a I'm a little wary of using the public type internally to represent what is logically a separate |
Blob is a pretty thin wrapper on top of the method exposed by Platform. We could address your concern by directly invoking the Platform API and representing Blobs internally as Strings. If we do that, I would advocate to still use the |
There was a problem hiding this comment.
After talking with Sebastian, I ended up keeping the ProtoByteString name and creating a thin class for it that mirrors Blob without the additional validation. The main issue I have with representing tokens as strings internally is that it is not type safe enough and makes it easy to introduce more bugs like this original one. We would have to remember to cast the string to each time we want to use it as a proto, even though type checking will allow us to pass the incorrect string representation.
By storing the tokens in an intermediate class, we are forced to convert the string to the correct platform representation in order to get the code to compile. Creating a separate ProtoByteString class also allows us to avoid using the public Blob type.
thebrianchen
changed the title
ProtoByteString to use Blob rather than String | Uint8ArrayProtoByteString to use its own class rather than String | Uint8Array
|
I’m definitely in favor of a separate type. At this point though it’s not really tied to protos so much though. Previously the union type really represented “types that protos will represent byte sequences as”. Now this is independent of that representation. Is suggest renaming to just |
| expect(actualTarget.resumeToken).to.equal(expectedTarget.resumeToken); | ||
| // actualTarget's resumeToken is a hexadecimal string, but the serialized | ||
| // resumeToken will be a base64 string, so we need to convert it back. | ||
| expect(actualTarget.resumeToken || '').to.equal( |
There was a problem hiding this comment.
See my comment below - can you verify that we are indeed using hexadecimal strings?
There was a problem hiding this comment.
We are using hexadecimal strings here because the spec tests pass in resume tokens as standard javascript hex strings. I guess what ends up happening is that btoa() still serializes and deserializes hex strings fine, which serves the purposes of the test.
There was a problem hiding this comment.
As discussed offline, these are all base64 encoded strings. Thanks for updating the comment.
| expect(actualTarget.resumeToken).to.equal(expectedTarget.resumeToken); | ||
| // actualTarget's resumeToken is a hexadecimal string, but the serialized | ||
| // resumeToken will be a base64 string, so we need to convert it back. | ||
| expect(actualTarget.resumeToken || '').to.equal( |
There was a problem hiding this comment.
As discussed offline, these are all base64 encoded strings. Thanks for updating the comment.
We've been storing tokens internally as
ProtoByteString, which is typed toString | Uint8Arrayin order to account for the fact that protos are sent as base64 strings and Uint8Arrays. However, casting between these 2 types is confusing and bug-prone, resulting in a latent bug where we were improperly decoding Uint8Arrays as storing them as strings. This PR separates creates a separateProtoByteStringclass to provide a type-safe representation for tokens.