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(TextEncoderStream): emoji encoding polyfill and add tests #6466

Merged
merged 4 commits into from
Jun 8, 2024

Conversation

octet-stream
Copy link
Contributor

@octet-stream octet-stream commented Jun 7, 2024

Overview

Fixes #6458

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests / types / typos

Description

The polyfill for TextEncoderStream introduced and proposed by me in #6310 (comment) for use in Bun and Cloudflare runtimes breaks emoji encoding. This PR fixes the encoding issue.

The issue caused by how I iterated over the input string: for...of loop relies on iterator protocol, which treats an emoji as a single character and iterates through it just once, and then String.charCodeAt(0) call was picking up just half of the symbol. Meanwhile, the Node.js' implementation iterates over the string using for loop which treats emojis as two symbols and then encodes each part of that symbol.

Use cases and why

Expected: The encoder stream polyfill should handle emoji symbols correctly.

Actual: The encoder stream polyfill breaks emoji symbols.

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • Added new tests to cover the fix / functionality

Copy link

netlify bot commented Jun 7, 2024

👷 Deploy request for qwik-insights pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 619d810

Copy link

pkg-pr-new bot commented Jun 7, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@builder.io/qwik (619d810)

npm i https://pkg.pr.new/@builder.io/qwik@6466

@builder.io/qwik-city (619d810)

npm i https://pkg.pr.new/@builder.io/qwik-city@6466

eslint-plugin-qwik (619d810)

npm i https://pkg.pr.new/eslint-plugin-qwik@6466

create-qwik (619d810)

npm i https://pkg.pr.new/create-qwik@6466

@octet-stream octet-stream marked this pull request as ready for review June 7, 2024 14:09
@octet-stream octet-stream requested review from a team as code owners June 7, 2024 14:09
@PatrickJS
Copy link
Member

@octet-stream thanks a ton for looking into this and getting the test for it

@PatrickJS PatrickJS enabled auto-merge (squash) June 7, 2024 18:33
@PatrickJS PatrickJS changed the title fix: Fix emoji encoding in TextEncoderStream polyfill and add tests fix(TextEncoderStream): emoji encoding polyfill and add tests Jun 7, 2024
@PatrickJS PatrickJS disabled auto-merge June 7, 2024 18:48
@PatrickJS PatrickJS enabled auto-merge (squash) June 7, 2024 19:44
Copy link
Member

@thejackshelton thejackshelton left a comment

Choose a reason for hiding this comment

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

PR Looks good to me!

Thanks for adding tests 🙌

Yeah let's wait for e2e tests to pass before we merge

Copy link
Member

@gioboa gioboa left a comment

Choose a reason for hiding this comment

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

Thanks for your help 👏🚀

@PatrickJS PatrickJS merged commit 82ae67e into QwikDev:main Jun 8, 2024
20 checks passed
@octet-stream octet-stream deleted the fix/text-encoder-polyfill branch June 8, 2024 10:14
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.

[🐞] TextEncoderStream polyfill breaks resumability
5 participants