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

Add enableSuspenseServerRenderer feature flag #13573

Merged

Conversation

alexmckenley
Copy link
Contributor

I've added a feature flag that will allow server rendering of placeholder components. This implementation will always render the fallback component when a Placeholder is encountered, but will be improved upon in the future.

@acdlite
Copy link
Collaborator

acdlite commented Sep 5, 2018

@alexmckenley Hey Alex! @sebmarkbage is going to work with you on the new streaming server renderer.

@pull-bot
Copy link

pull-bot commented Sep 5, 2018

Details of bundled changes.

Comparing: fb88fd9...99b6680

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js 0.0% 0.0% 645.08 KB 645.11 KB 151.15 KB 151.16 KB UMD_DEV
react-dom.development.js 0.0% 0.0% 640.32 KB 640.34 KB 149.75 KB 149.75 KB NODE_DEV
react-dom-server.browser.development.js +0.7% +0.4% 104.03 KB 104.78 KB 27.83 KB 27.95 KB UMD_DEV
react-dom-server.browser.development.js +0.7% +0.4% 100.17 KB 100.91 KB 26.86 KB 26.98 KB NODE_DEV
react-dom-server.node.development.js +0.7% +0.5% 102.09 KB 102.83 KB 27.38 KB 27.51 KB NODE_DEV
ReactDOM-dev.js 0.0% 0.0% 662.65 KB 662.72 KB 152.09 KB 152.1 KB FB_WWW_DEV
ReactDOMServer-dev.js +0.7% +0.5% 101.26 KB 101.98 KB 26.52 KB 26.64 KB FB_WWW_DEV
ReactDOMServer-prod.js 🔺+1.5% 🔺+0.6% 33.07 KB 33.57 KB 8.01 KB 8.06 KB FB_WWW_PROD

schedule

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
schedule.development.js n/a n/a 0 B 19.17 KB 0 B 5.74 KB UMD_DEV
schedule.production.min.js n/a n/a 0 B 3.16 KB 0 B 1.53 KB UMD_PROD
schedule-tracking.development.js +0.3% +0.1% 10.83 KB 10.86 KB 2.62 KB 2.63 KB NODE_DEV
ScheduleTracking-dev.js +0.7% +0.7% 10.43 KB 10.5 KB 2.35 KB 2.37 KB FB_WWW_DEV

Generated by 🚫 dangerJS

@@ -911,6 +916,27 @@ class ReactDOMServerRenderer {
this.stack.push(frame);
return '';
}
case REACT_PLACEHOLDER_TYPE: {
if (enableSuspenseServerRenderer) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if it's worth adding this to the existing renderer, even in this limited form. If we do, we should emit a warning so people know to migrate to the new one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The idea is to only enable this flag in the FB build for now since we're already using placeholders.

@acdlite
Copy link
Collaborator

acdlite commented Sep 5, 2018

Update: never mind, didn't realize you had discussed this with Sebastian already!

@@ -16,6 +16,7 @@ export const {
debugRenderPhaseSideEffects,
debugRenderPhaseSideEffectsForStrictMode,
enableGetDerivedStateFromCatch,
enableSuspenseServerRenderer,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Normally we would export const enableSuspenseServerRenderer = true; instead of this line. That way it should be enabled in www client but I don't know how we plan on using this file on the server in www. Perhaps we should configure the server build to just inline the feature flags in the build instead? Since this one is most meant for experiments based on GKs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was planning on controlling this in www like so: https://fburl.com/diff/i1l0949n.

let me know if you'd rather just hard-code the value here.

@sebmarkbage sebmarkbage merged commit 34348a4 into facebook:master Sep 5, 2018
jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
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

5 participants