-
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
Force GdprBanner not to render in SSR. #34462
Conversation
GdprBanner tries to access certain client-side-only properties, such as the `cookies` object in document, and navigator.userAgent. This caused SSR to fail when the component was added to the logged-out layout. Since it doesn't make sense to render the GDPR banner on the server anyway (since we don't have enough information to determine whether it should be shown), this change effectively disables it whenever the `document` object isn't present.
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~19 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. |
Thanks @sgomes for the fix. Does the SSR enter into action only when the logged-out page is served for a browser with JS disabled? Or are there other cases? Since this modification essentially disables the cookie banner I'm trying to get a sense of what percent of users will have the cookie banner disabled this way. If it's just for users with JS disabled I don't see any issue but if it's also for other users then it becomes a different proposition. Thank you! |
Hey, @MicBosi ! The SSR is always done. It's generated by the server and used as a first render, to give the user something to look at other than a blank screen. Once the user downloads the necessary JS, subsequent renders are then generated by their browser. This effectively means that users with JS enabled will see a simplified form with disabled controls first (the server-side rendered one), which gets replaced with the full experience once their JS kicks in. This full experience includes the cookie banner as normal. Users with JS disabled will only see the SSR experience which, as far as I know, won't even allow then to log in, due to the controls being disabled. Hope this helps! |
Thanks @sgomes for the explanation! Disabling the cookie banner seems completely safe then. |
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.
With JavaScript disabled, the form is rendered server-side, but it's not enabled. I assume that's to be expected since Calypso isn't much use without JavaScript, anyway. The GDPR banner isn't rendered.
With JavaScript enabled, the form is rendered and functional, and the GDPR banner is also rendered.
LGTM! 🚢
Thank you both for the review! 👍 |
This PR has one little issue that would be worth another patch. Components are expected to be isomorphic, i.e., the server renders exactly the same thing as the initial client-side render. If these renders are different, React can get angry about it and the resulting DOM markup can end up corrupted. After this patch, the server-side There are two solutions:
The first solution is a more correct one, but can be an overkill to implement. The second solution is kind of a hack, but very fast and efficient. |
Thanks, @jsnajdr ! I was worried about differing renders too, but that I never noticed any issues as I was testing this out (granted, it could be because it was all happening in production mode). Intuitively, I wouldn't expect this to be a problem, since it's a difference between "nothing" and "something", not "something" and "something else", but intuition isn't a good measure of correctness :) Is there any way we can know for sure before we add extra complexity to the component? |
We know for sure that the renders should be identical, and that not making them identical can lead to issues. Here's an example (with screenshot) of how the Login page got broken in the past due to rendering differences: #26944 (comment) |
It'd be best to have a more concrete notion of what types of mismatches are acceptable and which ones are not, but I'll accept just avoiding the risk in the first place. For option 1, we don't just need to modify |
GdprBanner
tries to access certain client-side-only properties, such asdocument
andnavigator.userAgent
.This caused SSR to fail when the component was added to the logged-out layout, which greatly affected the speed index of the login page.
It doesn't make sense to render the GDPR banner on the server anyway (since we don't have enough information to determine whether it should be shown), so this change effectively disables it by rendering nothing whenever the
document
object isn't present.Changes proposed in this Pull Request
GdprBanner
if thedocument
object isn't present.Testing instructions
/log-in
(without any URL parameters)Note that testing is complicated by the fact that not all environments have GDPR prompts enabled. Be sure that
gdpr-banner
is enabled in the environment you're testing with.