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

chore(qwik-city/middleware): add TextEncoderStream polyfill and tests #6310

Merged
merged 15 commits into from
May 17, 2024

Conversation

PatrickJS
Copy link
Member

@PatrickJS PatrickJS commented May 14, 2024

add _TextEncoderStream_polyfill and tests

Copy link

cloudflare-pages bot commented May 14, 2024

Deploying qwik-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 849a365
Status: ✅  Deploy successful!
Preview URL: https://54153751.qwik-8nx.pages.dev
Branch Preview URL: https://feat-polyfill-textencoderstr.qwik-8nx.pages.dev

View logs

@PatrickJS PatrickJS changed the title chore: _TextEncoderStream_polyfill chore(qwik-city/middleware): add TextDecoderStream and TextEncoderStream polyfill May 14, 2024
@PatrickJS PatrickJS marked this pull request as draft May 14, 2024 02:27
@PatrickJS PatrickJS marked this pull request as ready for review May 14, 2024 06:42
@PatrickJS PatrickJS changed the title chore(qwik-city/middleware): add TextDecoderStream and TextEncoderStream polyfill chore(qwik-city/middleware): add TextEncoderStream polyfill and tests May 14, 2024
Copy link

netlify bot commented May 16, 2024

Deploy Preview for qwik-insights ready!

Name Link
🔨 Latest commit 849a365
🔍 Latest deploy log https://app.netlify.com/sites/qwik-insights/deploys/6646a90c1d317900088cb1ff
😎 Deploy Preview https://deploy-preview-6310--qwik-insights.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@PatrickJS PatrickJS requested a review from wmertens May 16, 2024 17:26
@PatrickJS
Copy link
Member Author

@wmertens when you get time can you review

Copy link
Contributor

@octet-stream octet-stream left a comment

Choose a reason for hiding this comment

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

Saw this PR and decided to add a few bits after looking closer myself to TextEncoderStream. This implementation is shorter, and I believe it's fine to keep it as it is, but I have a few thoughts from what I understand:

  1. The TextEncoderStream class is not an extension of the TransformStream (you can see that from the documentation on MDN here), but it has same generic interface with readable and writable properties (see the spec here). I'm not sure if we are trying to achieve the most compatibility, but you can refer to Deno's and Node.js' implementations if that's the case.
  2. There are properties that I don't see on this class either. I left more comments on the specifics :)

packages/qwik-city/middleware/request-handler/polyfill.ts Outdated Show resolved Hide resolved
packages/qwik-city/middleware/request-handler/polyfill.ts Outdated Show resolved Hide resolved
packages/qwik-city/middleware/request-handler/polyfill.ts Outdated Show resolved Hide resolved
@PatrickJS
Copy link
Member Author

yeah the props are there only to match the previous polyfill. so everything is just-in-case

@octet-stream
Copy link
Contributor

octet-stream commented May 17, 2024

Oh, sure. But anyway here's TypeScript flavored version of TextEncoderStream polyfill based on Node.js' implementation, just in case:

/**
 * TextEncoderStream polyfill based on Node.js' implementation https://github.com/nodejs/node/blob/3f3226c8e363a5f06c1e6a37abd59b6b8c1923f1/lib/internal/webstreams/encoding.js#L38-L119 (MIT License)
 */
export class TextEncoderStream {
  #pendingHighSurrogate: string | null = null

  #handle = new TextEncoder()

  #transform = new TransformStream<string, Uint8Array>({
    transform: (chunk, controller) => {
      // https://encoding.spec.whatwg.org/#encode-and-enqueue-a-chunk
      chunk = String(chunk)

      let finalChunk = ""
      for (const item of chunk) {
        const codeUnit = item.charCodeAt(0)
        if (this.#pendingHighSurrogate !== null) {
          const highSurrogate = this.#pendingHighSurrogate

          this.#pendingHighSurrogate = null
          if (codeUnit >= 0xDC00 && codeUnit <= 0xDFFF) {
            finalChunk += highSurrogate + item
            continue
          }

          finalChunk += "\uFFFD"
        }

        if (codeUnit >= 0xD800 && codeUnit <= 0xDBFF) {
          this.#pendingHighSurrogate = item
          continue
        }

        if (codeUnit >= 0xDC00 && codeUnit <= 0xDFFF) {
          finalChunk += "\uFFFD"
          continue
        }

        finalChunk += item
      }

      if (finalChunk) {
        controller.enqueue(this.#handle.encode(finalChunk))
      }
    },

    flush: controller => {
      // https://encoding.spec.whatwg.org/#encode-and-flush
      if (this.#pendingHighSurrogate !== null) {
        controller.enqueue(new Uint8Array([0xEF, 0xBF, 0xBD]))
      }
    }
  })

  get encoding() {
    return this.#handle.encoding
  }

  get readable() {
    return this.#transform.readable
  }

  get writable() {
    return this.#transform.writable
  }

  get [Symbol.toStringTag]() {
    return "TextEncoderStream"
  }
}

@PatrickJS
Copy link
Member Author

looks good lets try it

@PatrickJS
Copy link
Member Author

@octet-stream thanks for finding this better polyfill

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.

Tests pass locally and make sense on my end.

My understanding, is that this polyfill is removing the previous one, and is a more robust version that should support the 5% or so where this is not supported.

Is that the case? Anything I should look in-depth into?

Otherwise LGTM! 🙌

@PatrickJS PatrickJS merged commit a2f3739 into main May 17, 2024
28 checks passed
@PatrickJS PatrickJS deleted the feat-polyfill-TextEncoderStream branch May 17, 2024 01:42
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.

None yet

3 participants