-
Notifications
You must be signed in to change notification settings - Fork 2k
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
SSR Footer: Use an empty state for Automattic branding before set the final value #77916
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~28 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
packages/wpcom-template-parts/src/universal-footer-navigation/index.tsx
Outdated
Show resolved
Hide resolved
…l state for components" This reverts commit 9a94552.
7cbafea
to
d5079f5
Compare
This PR modifies the release build for happy-blocks To test your changes on WordPress.com, run To deploy your changes after merging, see the documentation: PCYsg-r7r-p2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for addressing all feedback 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me too, and works as described.
const automatticBranding = useAutomatticBrandingNoun(); | ||
const [ automatticBranding, setAutomatticBranding ] = useState( { article: '', noun: '' } ); | ||
|
||
useLayoutEffect( () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way useLayoutEffect
has been causing warnings in the React 18 PR #77046. There's more info about it here: https://gist.github.com/gaearon/e7d97cdf38a2907924ea12e4ebdf3c85
But the core idea is that since effects don't run on the server in SSR, useLayoutEffect
can be problematic because its purpose is to update content before people see it. When SSR generates a layout without using the effect, that content wouldn't include what the layout effects expects to add.
Thankfully, this isn't a problem for us because the default content is just the Automattic logo, so we have a graceful fallback.
What I did to fix the warning in #77046 was to conditionally define useLayoutEffect
in SSR mode like this:
const useIsomorphicEffect = typeof window !== 'undefined' ? useLayoutEffect : useEffect;
And it seems to resolve the issue. Just wanted to drop a comment here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly, it looks like this warning will be removed in a future React version: facebook/react#26395
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This particular code would be better off if it used just useEffect
. Without SSR, the benefit of useLayoutEffect
would be that the version of the page with the incomplete footer would never be actually painted. The first paint would occur only after the layout effect sets the footer to the final random value.
But with SSR, the initial version is always painted, because it's a part of the initial HTML markup. useLayoutEffect
doesn't make any difference compared to useEffect
.
Related to #77696
Fixes #77799
Proposed Changes
SSR requires the initial state to be the same for the client and server version, thus having a random text on the footer makes it always use the client version.
In order to align the initial state for both cases, the initial state for the Automattic branding will be only the "Automattic" part, and the text will be added later.
Testing Instructions
Trunk
/theme/miniature
console
and you SHOULD see an error stating there is a mismatch between the client and server version and you'll see the client version on the screenThis branch
/theme/miniature
Automattic
at first, and then the correct brandingconsole
and you SHOULD NOT see any errorsPre-merge Checklist