Boot: Don't render empty Layout

[ockham]

This is a left-over that we must've missed when enabling single-tree rendering.

All of Calypso's client-side page() route definitions now include makeLayout and render middleware, so rendering an empty Layout component during bootstrapping is obsolete.

Discovered while working on, and needed for #26930.

Best viewed without whitespace changes.

To test:

• Verify that Calypso renders and works properly, when landing in different sections
• Be sure to include some logged-out and SSR'd sections

[matticbot]

Test live: https://calypso.live/?branch=remove/boot-empty-layout

[ockham]

I've retained the ! document.getElementById( 'primary' ) criterion here, since the catch-js-errors stuff was originally wrapped in it, but I have a hunch that it's not really required for it.

Of note, document.getElementById( 'primary' ) isn't always false -- isomorphic sections (such as /themes) render a component hierarchy on the server side, so the client will find a #primary div. (However, the rationale found in the PR desc still holds -- all individual sections render their own Layout component hierarchy, so we don't need to render an empty Layout here.)

[alisterscott]

The e2e tests don't like this branch - I am trying to work out why.

One thing that stands out is the notifications panel doesn't work:

1. Load https://calypso.live/?branch=remove/boot-empty-layout
2. Choose the notifications icon:

[ockham]

Thanks @alisterscott, looking into this now...

[ockham]

The 'Write Post' button has a similar issue, where rather than just opening the sites dropdown, it takes the user right away to the post editor.

[ockham]

Weird. Removing that empty Layout component seems to screw up React's event system somehow -- looks like event.preventDefault() and event.stopPropagation() are silently ignored now...

[jsnajdr]

I stumbled upon the same issue even without this patch applied, when working on Webpack CSS for the /log-in section. Clicking on the notification icon in masterbar navigates to /notifications instead of toggling the notifications panel.

What's going on here? The clicked element is a link that has both href and an onClick handler:

<a href="/notifications" onClick={ toggleNotesFrame } />

The theory is that toggleNotesFrame calls event.preventDefault() and event.stopPropagation(), so the browser's default handler doesn't navigate to the href and user-space handlers on parent elements (e.g., document.addEventListener( 'click' )) are not called either.

In practice, there are two complications:

page.js adds a global click listener to document that intercepts all clicks on <a href> elements and instead of letting the browser navigate to the href (that would cause page reload) does the navigation with page.show (internal SPA routing without page reload).

As the event first goes through <a onClick> and only then bubbles up to the document, the page.js handler shouldn't be called at all (because stopPropagation) and even if it was called, shouldn't do the navigation as it checks for e.defaultPrevented, right?

There is a second complication that pierces both defenses 😄

In React, <a onClick> doesn't mean that the actual <a> DOM element has a click DOM listener. React event system does something different: it attaches only one global DOM listener to document and then dispatches the event to the right component (thanks to e.target).

So, in DOM, we have two click listeners on document. One from page.js, one from React. They are called in the order they were registered.

If the React one was registered first and page.js second, everything works as expected. If page.js was first, it will do the navigation on click and the preventDefault and stopPropagation in the React handler comes too late.

This patch changes the order of these listeners, as it removes the renderLayout call. The first-ever React render now happens only after page.start installs its click handler.

To verify, place breakpoints inside page.js's page.start function and inside trapBubbledEvent function in react-dom.development.js. And watch in what order they are triggered.

The bug can happen even without this patch applied, when two conditions hold:

• renderLayout is not called, e.g., when DOM already has a #primary element from SSR
• there is an <a href onClick> element where onClick calls preventDefault/stopPropagation in order to prevent the href navigation.

[ockham]

Great sleuthing, thanks a lot, Jarda! The page.js intercepting onClick had raised my suspicion, but I hadn't done anything to back up that theory.

[ockham]

Makes me wonder about potential fixes:

• Move the page.start() call to the render() middleware? Tho page call inside middleware inside page call sounds... problematic
• Or is this what the dispatch option is for?

Edit: Neither works 🙁

[ockham]

Notifications and subscribe button work with this patch

diff --git a/client/boot/app.js b/client/boot/app.js
index 5db2d2c..a98ee6b 100644
--- a/client/boot/app.js
+++ b/client/boot/app.js
@@ -33,7 +33,7 @@ const boot = currentUser => {
                setupMiddlewares( currentUser, reduxStore );
                invoke( project, 'setupMiddlewares', currentUser, reduxStore );
                detectHistoryNavigation.start();
-               page.start( { decodeURLComponents: false } );
+               page.start( { click: false, decodeURLComponents: false } );
        } );
 };
 

But that would mean losing client-side routing, which we obviously don't want to lose!

[jsnajdr]

Move the page.start() call to the render() middleware? Tho page call inside middleware inside page call sounds... problematic

I think that page.start should be always called first and only then the first React render should happen. That's the "natural" order. To render a meaningful page content, we need to know which route we are on and then render the route. Therefore, the router needs to start first.

The renderLayout renders an empty route-independent placeholder, that's the only reason it can work before page.start. And it's undesirable for SSR, of course:

1. show index.html with SSRed content for the requested route
2. renderLayout resets the content back to placeholder with big "W" 👎
3. page.start triggers route handling
4. client-side React renders the same content as 1. (this step called "hydration")

Additionally, React 17 plans to become grumpy when the SSR content doesn't exactly match the client-rendered content when hydrating. (link)

Notifications and subscribe button work with this patch

Yes, I think that removing the global click handler is the right way forward. But it's not a one-line change. We'll need to replace the hacked <a href onClick> elements with a Link component similar to what react-router has.

[ockham]

Move the page.start() call to the render() middleware? Tho page call inside middleware inside page call sounds... problematic

I think that page.start should be always called first and only then the first React render should happen. That's the "natural" order. To render a meaningful page content, we need to know which route we are on and then render the route. Therefore, the router needs to start first.

Yeah, makes sense. I kinda expected something like that, but I found page.js docs weren't super clear -- they sounded like page.start() might only be needed to set up that auto-injected onClick handler.

Notifications and subscribe button work with this patch

Yes, I think that removing the global click handler is the right way forward. But it's not a one-line change.

Yeah, I'm fully aware that we can't just drop client side handling and call it a day 😅

We'll need to replace the hacked <a href onClick> elements with a Link component similar to what react-router has.

Yep, that's going to be "fun".

I mean there might be other ways, like setting up page.js with click: false, and adding an auto-injected onClick handler ourselves after rendering, so the React event handling system will take priority. (Granted, I'm not entirely sure that'll work.)

---

Well, #26930 is just in exploratory state right now, and even if we decide to move forward with it, I can pass a wpRegistry prop to the empty Layout we render in bootstrapping, so this isn't super crucial for me.

[jsnajdr]

I mean there might be other ways, like setting up page.js with click: false, and adding an auto-injected onClick handler ourselves after rendering

Yes, that's going to work and seems to be the easiest way to unblock this. Init page.js with click: false, and then add a click listener to Layout:

<div className="layout" onClick={ stealAhrefClicks }>

The handler will be the same. Unfortunately page.js doesn't export it, so we'll need to copy&paste it. As the click hander is now part of the React event system, bubbling from target up the element hierarchy will work as expected.

Do we use Layout to render 100% of our routes?

The best long term solution is still a react-router-like Link component.

[sirreal]

Do we use Layout to render 100% of our routes?

There are at least Layout and LayoutLoggedOut.

[dmsnell]

It sounds like we should also update the notifications HTML - it's not an <a> (anymore) in reality and maybe we should use a <button> instead. Would that help this or do we still have to deal with the click trickery?

[blowery]

We'll need to watch out for react-dom changes that are coming too. See facebook/react#2043 and facebook/react#13525

[ockham]

Aside, love it when a 'cleanup'/'janitorial' PR reveals that we're relying on an oversight for things to work 🙄 https://xkcd.com/1172/

[ockham]

We'll need to watch out for react-dom changes that are coming too. See facebook/react#2043 and facebook/react#13525

Thanks Ben. In facebook/react#13525, they're framing facebook/react#2043 to be specifically about stopPropagation and friends. Makes me wonder if we should wait for them since that might solve our problems (and continue to carry the workaround with us until then)? Tho that's planned for post-17.0.0 it seems? Could take a while...

[matthewp]

I've released that as page.js 1.11.1: https://github.com/visionmedia/page.js/releases/tag/1.11.1 . Thanks a lot for your patience with me and for the contribution.

[alisterscott]

I tested this on https://calypso.live/?branch=remove/boot-empty-layout and also locally and the whole page view flashes blank on every load - so for example the sidebar flashes on every click - this is most obvious when you use mobile sized views.

This is the reason the e2e tests are failing also.

[alisterscott]

Issues with rendering and flashing on testing

[jsnajdr]

the whole page view flashes blank on every load - so for example the sidebar flashes on every click

That's because this PR relies on page.js being upgraded to 1.11.1. I pushed a commit that does the upgrade. Let's check if things are fixed now.

[alisterscott]

The e2e tests are failing because a masterbar doesn't load on the logged out base version of this URL: https://hash-7d23fc0077b078359949da3bc93c7a814783cf78.calypso.live/?apppromo

Compared to other branches which show the masterbar:

https://hash-80ba41927f2074836f28d5bc414086d685013b6d.calypso.live/?apppromo

[jsnajdr]

The e2e tests are failing because a masterbar doesn't load on the logged out base version of this URL

Yes, apparently there is no page.js handler that would handle the logged out version of /. Logged in, it's Reader, but logged out, we were relying on the renderLayout call that's being removed in this PR.

[jsnajdr]

The e2e tests are failing because a masterbar doesn't load on the logged out base version of this URL

Pushed a simple fix in e5b64d2.

Eventually such logged-out routes should redirect to login page. Folks have been doing that for selected individual pages that are target of links from emails (campaings, notifications): /stats, /google-my-business/new, many Reader routes... See #25498 or #25741 for example. These all feel like quick hotfixes rather than a systematic approach.

Anyway, let's see if the tests are more happy now 🤞

[jsnajdr]

All tests are 💚 now -- ready for final 👀 and 🚢ping

No longer valid

[alisterscott]

Tests well and all e2e tests passing 👍

[jsnajdr]

I had to revert this because SSR of the login page breaks:

See also p1541402019129800-slack-calypso

Apprarently React is very sensitive when it hydrates a server-rendered markup on the client and the markup on both sides is not exactly the same. This PR introduced some subtle change that breaks the hydration. To be debugged.

[alisterscott]

I had to revert this because SSR of the login page breaks:

😬

Is that every log in page?

I'm guessing the page is still functional just horribly rendered considering all our e2e tests passed and they use that page

[jsnajdr]

@alisterscott I didn't debug this issue yet, so I don't know why it's happening. The rendered page is good enough for e2e tests to pass. The form fields and buttons are there and are active. But the HTML markup is corrupted and the rendering and styling is broken.

I suspect that the ReactDOM.hydrate code from @ockham's #27548 is wrong. hydrate should be used if and only if the page was really server-rendered, but when there is a redirect query param, the login page is not server-rendered? That's the first hypothesis that needs to be verified.