Skip to content

Conversation

@jplhomer
Copy link
Contributor

@jplhomer jplhomer commented May 31, 2022

Summary

This is because not all streaming implementations (e.g. Cloudflare) respect the default behavior of settings highWaterMark to 0 for byte streams: 75662d6

Related Hydrogen bug: Shopify/hydrogen#1383

Being explicit guarantees the intended behavior across runtimes. I've contacted Cloudflare, and they are working on fixing this in their implementation in the meantime.

How did you test this change?

  • Added the highWaterMark manually in the compiled source code
  • Deployed to e.g. Cloudflare and observed the intended results

This is because not all streaming implementations respect the
default behavior of settings highWaterMark to 0 for byte streams.
Being explicit guarantees the intended behavior across runtimes.
@sizebot
Copy link

sizebot commented May 31, 2022

Comparing: 1328ff7...fff3966

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 131.28 kB 131.28 kB = 42.13 kB 42.13 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 136.54 kB 136.54 kB = 43.68 kB 43.68 kB
facebook-www/ReactDOM-prod.classic.js = 439.35 kB 439.35 kB = 80.29 kB 80.29 kB
facebook-www/ReactDOM-prod.modern.js = 424.64 kB 424.64 kB = 78.13 kB 78.13 kB
facebook-www/ReactDOMForked-prod.classic.js = 439.35 kB 439.35 kB = 80.29 kB 80.29 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against fff3966

@sebmarkbage
Copy link
Contributor

You're getting failed tests due the size() function. Is that needed?

@jplhomer
Copy link
Contributor Author

Ahh, Flow doesn't like any queuingStrategy without a size method, but the runtime/polyfill does not like a byte-stream that has a queuing strategy with a size method.

I've updated with $FlowFixMe.

I tried implementing https://developer.mozilla.org/en-US/docs/Web/API/ByteLengthQueuingStrategy but Flow doesn't seem to recognize that as a valid thing, either.

@sebmarkbage sebmarkbage merged commit 26a5b3c into facebook:main May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants