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: iconVersionId wrong encode decode #156

Merged
merged 12 commits into from
Oct 10, 2023

Conversation

tomasciccola
Copy link
Contributor

@tomasciccola tomasciccola commented Oct 9, 2023

on src/lib/{encode,decode}-conversions.js when converting the iconVersionId for IconVariant we where encoding//decoding the string as 'hex'.
Since the strings has a slash ('/'), we where losing it when encoding the document.
Tests weren't failing since we weren't generating a valid versionId (without the /${index})

  • Check if doing Buffer.from(string, 'utf-8') and buf.toString('utf-8') is safe

@tomasciccola tomasciccola requested a review from achou11 October 9, 2023 19:21
@tomasciccola tomasciccola self-assigned this Oct 9, 2023
@tomasciccola tomasciccola changed the title Chore/fix icon wrong encode decode Chore: fix icon wrong encode decode Oct 9, 2023
@tomasciccola tomasciccola changed the title Chore: fix icon wrong encode decode fox: iconVersionId wrong encode decode Oct 9, 2023
@tomasciccola tomasciccola changed the title fox: iconVersionId wrong encode decode fix: iconVersionId wrong encode decode Oct 9, 2023
@achou11
Copy link
Member

achou11 commented Oct 9, 2023

Not super sure about the changes here. Makes me wonder if the blobVersionId field should be derived similarly to how we derive versionId in the Common schema.

That has a links field, where a Link is { coreDiscoveryKey: Buffer, index: number }

@tomasciccola
Copy link
Contributor Author

Makes me wonder if the blobVersionId field should be derived similarly to how we derive versionId in the Common schema.

Yeah, I think that makes a lot of sense. I made it those changes on the last commit but now I have a type error because Icon.blobVersionId is typed as optional on the protobuf typing.
I thought putting [(required) = true] makes the field required (in some sense...). Is there smth that I'm missing? 'cause giving that field a default value doesn't make much sense... @gmaclennan any pointers?

Co-authored-by: Gregor MacLennan <[email protected]>
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.

3 participants