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

Explicitly set highWaterMark to 0 for ReadableStream #24641

Merged
merged 2 commits into from May 31, 2022

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
Collaborator

sebmarkbage commented May 31, 2022

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

@jplhomer
Copy link
Contributor Author

jplhomer commented May 31, 2022

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
36 checks passed
facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Jun 6, 2022
Summary:
This sync includes the following changes:
- **[dd4950c90](facebook/react@dd4950c90 )**: [Flight] Implement useId hook ([#24172](facebook/react#24172)) //<Josh Story>//
- **[26a5b3c7f](facebook/react@26a5b3c7f )**: Explicitly set `highWaterMark` to 0 for `ReadableStream` ([#24641](facebook/react#24641)) //<Josh Larson>//
- **[aec575914](facebook/react@aec575914 )**: [Fizz] Send errors down to client ([#24551](facebook/react#24551)) //<Josh Story>//
- **[a2766387e](facebook/react@a2766387e )**: [Fizz] Improve text separator byte efficiency ([#24630](facebook/react#24630)) //<Josh Story>//
- **[f7860538a](facebook/react@f7860538a )**: Fix typo in useSyncExternalStore main entry point error ([#24631](facebook/react#24631)) //<François Chalifour>//
- **[1bed20731](facebook/react@1bed20731 )**: Add a module map option to the Webpack Flight Client ([#24629](facebook/react#24629)) //<Sebastian Markbåge>//
- **[b2763d3ea](facebook/react@b2763d3ea )**: Move hydration code out of normal Suspense path ([#24532](facebook/react#24532)) //<Andrew Clark>//
- **[357a61324](facebook/react@357a61324 )**: [DevTools][Transition Tracing] Added support for Suspense Boundaries ([#23365](facebook/react#23365)) //<Luna Ruan>//
- **[2c8a1452b](facebook/react@2c8a1452b )**: Fix ignored setState in Safari when iframe is touched ([#24459](facebook/react#24459)) //<dan>//
- **[62662633d](facebook/react@62662633d )**: Remove enableFlipOffscreenUnhideOrder ([#24545](facebook/react#24545)) //<Ricky>//
- **[34da5aa69](facebook/react@34da5aa69 )**: Only treat updates to lazy as a new mount in legacy mode ([#24530](facebook/react#24530)) //<Ricky>//
- **[46a6d77e3](facebook/react@46a6d77e3 )**: Unify JSResourceReference Interfaces ([#24507](facebook/react#24507)) //<Timothy Yung>//
- **[6cbf0f7fa](facebook/react@6cbf0f7fa )**: Fork ReactSymbols ([#24484](facebook/react#24484)) //<Ricky>//
- **[a10a9a6b5](facebook/react@a10a9a6b5 )**: Add test for hiding children after layout destroy ([#24483](facebook/react#24483)) //<Ricky>//
- **[b4eb0ad71](facebook/react@b4eb0ad71 )**: Do not replay erroring beginWork with invokeGuardedCallback when suspended or previously errored ([#24480](facebook/react#24480)) //<Josh Story>//
- **[99eef9e2d](facebook/react@99eef9e2d )**: Hide children of Offscreen after destroy effects ([#24446](facebook/react#24446)) //<Ricky>//
- **[ce1386028](facebook/react@ce1386028 )**: Remove enablePersistentOffscreenHostContainer flag ([#24460](facebook/react#24460)) //<Andrew Clark>//
- **[72b7462fe](facebook/react@72b7462fe )**: Bump local package.json versions for 18.1 release ([#24447](facebook/react#24447)) //<Andrew Clark>//
- **[22edb9f77](facebook/react@22edb9f77 )**: React `version` field should match package.json ([#24445](facebook/react#24445)) //<Andrew Clark>//
- **[6bf3deef5](facebook/react@6bf3deef5 )**: Upgrade react-shallow-renderer to support react 18 ([#24442](facebook/react#24442)) //<Michael サイトー 中村 Bashurov>//

Changelog:
[General][Changed] - React Native sync for revisions bd4784c...d300ceb

jest_e2e[run_all_tests]

Reviewed By: cortinico, kacieb

Differential Revision: D36874368

fbshipit-source-id: c0ee015f4ef2fa56e57f7a1f6bc37dd05c949877
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.

None yet

4 participants