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

feat: allow direct base64 call against HandleValue #69

Merged
merged 2 commits into from
Jun 25, 2024

Conversation

guybedford
Copy link
Contributor

Adds back the ability to call convertJSValueToByteString for a HandleValue by declaring the header for the original function variant. Necessary for Fastly's statusText setter to pass the web platform tests, because converting to a string first was altering unsupported characters.

Copy link
Member

@tschneidereit tschneidereit left a comment

Choose a reason for hiding this comment

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

I hadn't really looked at this code before. Obviously adding this declaration makes sense, and I'm okay with landing this as-is.

Ideally though, it'd be great to rename at least the other export, or perhaps both. I think something like ToJSByteString would work well for both.

Also, it took me a bit to understand what this does: I first thought it'd base64 encode the input, where really what it does is coerce the input value to a latin1 string iff the result of stringifying it allows for that to happen losslessly. Having a comment explaining that would be great.

However, none of that needs to block landing this if you'd prefer getting on with things.

@guybedford
Copy link
Contributor Author

I've renamed these to valueToJSByteString and stringToJSByteString respectively, let me know if that works for you.

@tschneidereit
Copy link
Member

Sure, yeah. I think we could give them the same name and rely on overloading resolution, but this works, too.

@guybedford guybedford merged commit af6c533 into main Jun 25, 2024
1 check passed
@guybedford guybedford deleted the base64-string-handle branch June 25, 2024 20:10
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.

2 participants