Theme Showcase: Visiting the showcase while logged in forces you to select a site.

[mreishus]

Changes proposed in this Pull Request

• Visiting /themes while logged in forces you to select a site

Testing instructions

• Visit /themes while logged out
  • Should still see the themes showcase
• Visit /themes while logged out and javascript is off
  • Should still see the themes showcase (Server side rendered)
• Visit /themes while logged in
  • You should be forced to choose a site to proceed, and end up on a url like /themes/<siteurl>.

Dev Issues

Note: I am seeing some weirdness on the site selection page. However, I believe it is an artifact of isometric rendered pages (server side + client side) in the current dev environment. Screenshot:

I don't believe it is related to this branch, because I see the same thing on the current trunk commit with no changes:

Notice production renders fine, but in dev, the same page under the same conditions does not render properly. I believe there is some sort of bug when the client side render 'takes over' the server side render.

In short, without any changes in this PR, I am seeing /themes render quite differently in prod vs. localhost dev while logged in. Anyone know any more about this? (server-side-rendering.md mentioned these gurus: @ehg @mcsf @ockham @seear)

Edit: Reasoning for this discrepancy is explained here: pd2qbF-P-p2

To temporarily fix the issue for localhost testing, also apply this patch to your local installation:

diff --git a/client/controller/web-util.js b/client/controller/web-util.js
index 55d62fe50e..20940e794b 100644
--- a/client/controller/web-util.js
+++ b/client/controller/web-util.js
@@ -8,5 +8,5 @@ export function render( context ) {
 }
 
 export function hydrate( context ) {
-       ReactDom.hydrate( context.layout, document.getElementById( 'wpcom' ) );
+       ReactDom.render( context.layout, document.getElementById( 'wpcom' ) );
 }

A larger fix for this is in the works in #52731

Related to #14896

[github-actions]

Test live: https://calypso.live/?branch=update/themes-forces-site-selection

[matticbot]

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~1585 bytes removed 📉 [gzipped])

name    parsed_size           gzip_size
theme      -16869 B  (-5.5%)    -4040 B  (-4.6%)
themes      -6320 B  (-1.6%)    -1548 B  (-1.3%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

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.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

[mreishus]

This PR is causing the e2e tests to fail, but in a unique way: They don't even build. This seems to be repeatable and unique to this branch, that is, it represents a real problem with this changeset. Currently unsure why.

  #15 18.87 [fallback ] <s> [webpack.Progress] 37% building 0/4 entries 16100/16162 dependencies 2520/4130 modules
09:43:08
  #15 18.95 Error: package not found: react-native, dependency of ./node_modules/react-native-url-polyfill
09:43:08
  #15 18.95     at processPackage (/calypso/bin/copy-production-modules.js:99:10)
09:43:08
  #15 18.95     at processPackage (/calypso/bin/copy-production-modules.js:109:3)
09:43:08
  #15 18.95     at processPackages (/calypso/bin/copy-production-modules.js:38:3)
09:43:08
  #15 18.95     at Object.<anonymous> (/calypso/bin/copy-production-modules.js:21:26)
09:43:08
  #15 18.95     at Module._compile (internal/modules/cjs/loader.js:1063:30)
09:43:08
  #15 18.95     at Object.Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
09:43:08
  #15 18.95     at Module.load (internal/modules/cjs/loader.js:928:32)
09:43:08
  #15 18.95     at Function.Module._load (internal/modules/cjs/loader.js:769:14)
09:43:08
  #15 18.95     at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)
09:43:08
  #15 18.95     at internal/main/run_main_module.js:17:47
09:43:08
  #15 18.96 error Command failed with exit code 1.

[mreishus]

I'm able to recreate it locally by running docker build --build-arg workers=8 --build-arg node_memory=32768 --build-arg use_cache=true -f Dockerfile .. However, I did need to set this up in my environment, to access the special a8c proxy which is used to contact our registry.

Create directory+file /etc/systemd/system/docker.service.d/http-proxy.conf:

[Service]
Environment="HTTP_PROXY=socks5://127.0.0.1:8080/"
Environment="HTTPS_PROXY=socks5://127.0.0.1:8080/"

Then systemctl daemon-reload && systemctl restart docker as root.

[mreishus]

The docker build starts breaking once I add this line:

		return sites( context, next );

to

export function loggedIn( context, next ) {

Not sure why.

[mreishus]

Cause (somewhat) discovered: Importing the sites middleware from client/my-sites/controller.js causes that file to be processed by bin/copy-production-modules.js during the docker build process. client/my-sites/controller.js includes import { removeQueryArgs } from '@wordpress/url';.

@wordpress/url depends on react-native-url-polyfill, which depends on react-native, which cannot be included during this portion of the build process and causes an error. So I need a way to bring in the sites middleware without using any files that import @wordpress/url (or any other files that might have a dependency chain on react-native).

[mreishus]

Fixed here:
[update/themes-forces-site-selection 04e26f0a0d] Extract sites() middleware to its own file 04e26f0a0d
Ready for re-review.

[kwight]

Great, works as described. I'm not the best to review all the JS, but your explanation of extracting the middleware makes sense.

I saw one console error which isn't visible on prod (the first "Visit /themes while logged out" case), but prod looked so laughably different I'm not sure it's a concern. 🙃

There's one seemingly related test still failing; I wonder if it needs a site to be specified ([WPCOM] Themes: All sites (mobile): Preview a theme).

[mreishus]

I saw one console error which isn't visible on prod (the first "Visit /themes while logged out" case)

Seeing the same error message. The mechanism makes sense:

• aria-controls inside search is rendered with an instanceId, which comes from a random uuid.
• The server side rendered version will create one random uuid, and the client hydrated version will create a second random uuid, which don't match, but hydrate() expects the rendered content to be identical between the server and client.

I'm not sure how to fix, or why the error isn't happening when I visit https://wordpress.com/themes while logged out; this does the same SSR Html -> Client Hydrate progression. Checking on production, I looked at the initial server rendered html sent over the wire with the network tab and saw <div role="button" class="search__icon-navigation" aria-controls="search-component-43f07e51-fc52-4b53-84d3-955e8640762e", then checked $(".search__icon-navigation") in the console and saw the exact same uuid in aria-controls! I don't know why this matches; I would have expected the client side hydration to take over and choose a different UUID. Maybe @jsnajdr can advise if this is something we need to worry about or how to fix?

There's one seemingly related test still failing; I wonder if it needs a site to be specified ([WPCOM] Themes: All sites (mobile): Preview a theme).

This makes sense, I'll check out the tests and look to update.

[mreishus]

Updated tests

• wp-theme-multisite-spec.js used to have two tests: Previewing and activating. Both used an account with multiple sites.
  • Previewing works completely the same except it starts out by visiting /themes/mysite.word press.com instead of /themes.
  • Activating used to visit /themes, press activate, then look for a site selector and choose a site there. Now it visits /themes/mysite.word press.com, presses activate, and looks for a success modal.

• I wanted to add a third test that visits `/themes/ and checks that the "Choose a site" page loads, but I had trouble getting the e2e tests to work against localhost. Asked in slack for help. This code is currently commented out.

• Working on the tests made me realize I should probably test going to /themes with an account that only has one site. I'll check that out tomorrow.

• The test file is still called multisite-spec.js .. unsure if this needs changing.

[jsnajdr]

If you're removing the multi-site Themes view completely, you can also remove the ThemesSiteSelectorModal component and likely some others, too.

[jsnajdr]

aria-controls inside search is rendered with an instanceId, which comes from a random uuid.

The random uuids were introduced by @tyxla in #52673 (they were incrementing counters from lodash before) and since then, the IDs are guaranteed to be different in the client and server renders.

Before that, both IDs were usually 0 and they were the same only by accident. Server called _.uniqueId only at that one place, got 0, and then cached the response and returned the cached markup with 0 on every /themes request. Client called _.uniqueId also only once (or at least the call in the Search component was the first one) and got 0.

The best solution would be a context provider for instance IDs that keeps the ID counter local for a React tree. Then, if server and client really render the same thing, the IDs would be allocated in the same order and would be the same on both server and client: facebook/react#5867 (comment)

Good improvement idea for useInstanceId from @wordpress/compose.

[jsnajdr]

The trouble with importing sites could be solved by moving the loggedIn handler to a separate module that is never imported on server. I tried to do that in #52813.

Rendering the site selector can be done, arguably more elegantly, with two distinct route handlers:

page( '/themes', siteSelection, sites, render );
page( '/themes/:site', siteSelection, loggedIn, navigation, render );

This approach keeps each handler focused on exactly one task, and prevents rendering sidebar (navigation) in the site selection UI, which is currently a bug in this PR:

[mreishus]

Next week: Needs rebasing off of #52813 and re-examination

[mreishus]

Reworked to use additional routes, one batch calling loggedIn(), another batch calling sites().
(The old method had loggedIn() calling sites() sometimes, should be available on ed34398 and parents, now removed from this branch via rebasing. Worst case scenario I have a diff w/ the old changes on my HD).

Ready again for review: @jsnajdr @Automattic/serenity

• I also need to check this with a user that only has one site.

[jsnajdr]

You probably wanted to check ! context.params.site_id, but I think this check is redundant anyway: the presence of site_id is guaranteed by the route definition, and can be removed as a debug message that's no longer needed.

[mreishus]

Yea, it was supposed to be negative

the presence of site_id is guaranteed by the route definition,

This is true. I was mostly thinking of the next developer to come around and work on routes. They might not realize that the loggedIn() function assumes that a site_id is present, start rearranging the routes and middleware, and violate an assumption that isn't obvious.

[jsnajdr]

The fetch and validate handlers can be removed. We can show the site selector even for un-validated vertical and filter parameters, and verify them only after navigating to the site_id-enabled route.

[jsnajdr]

We could also remove the isLoggedIn check from the code that installs the route handlers and unify them into a single handler set:

router(
  routesWithSites,
  redirectToLoginIfLoggedOut,
  fetchAndValidateVerticalsAndFilters,
  siteSelection,
  loggedIn,
  navigation,
  makeLayout
);

router(
  routesWithoutSites,
  selectSiteIfLoggedIn,
  fetchAndValidateVerticalsAndFilters,
  loggedOut,
  makeLayout
);

• routes with sites are for logged-in users. Logged-out users are redirected to login.
• routes without sites are for logged-out users. Logged-in users are shown asite selector and then to a site-specific route.

Each handler has an "escape hatch" at the beginning and then proceeds to render the main case.

[jsnajdr]

routesWithSites could have a dedicated handler here, which does a simple:

router( routesWithSites, redirectToLoginRightAway );

If I'm logged out and go to a route with site_id, a redirect to login is guaranteed.

[jsnajdr]

handlers are under no obligation to call next()

Yes that's true. I was finally able to wire up a composeHandler helper:

function composeHandlers( ...handlers ) {
  return ( context, next ) => {
    const it = handlers.values();
    function handleNext() {
      const nextHandler = it.next().value;
      if ( ! nextHandler ) {
        next();
      } else {
        nextHandler( context, handleNext );
      }
    }
    handleNext();
  };
}

(instead of reduceRight or reduce I ended up using recursion)

It could be used in the selectSiteIfLoggedIn handler like this:

composeHandlers( siteSelection, sites, makeLayout, clientRender )( context );

Or for another handler:

export const fetchAndValidateVerticalsAndFilters = composeHandlers(
  fetchThemeFilters,
  validateVertical,
  validateFilters
);

[jsnajdr]

Looks good and works well 👍 I think this ended up being a very elegant refactoring that clarifies the very complex route handling in /themes.

[kwight]

Great work @mreishus ! 🚀

[mreishus]

Rebasing to solve conflicts with some e2e test refactors and renaming on trunk, then looking how the e2e test run now. I can't help but feel that a merge based workflow inside this PR would have made this much easier (one conflict resolution vs one at each step), and that more history would be preserved.

[mreishus]

I ended up with

import { By } from 'selenium-webdriver';
import * as driverHelper from '../lib/driver-helper';

which I definitely don't want - the rebase did not apply my changes to the renamed files (IE, I would have expected 7349b78 to be applied to the renamed wp-theme__multisite-activate-spec.js - but possibly not, because my previous change to wp-theme__multisite-activate-spec.js was implemented as a rebase conflict resolution) . Investigating.

Edit: Fixed.

[mreishus]

Is Pull Request / ICFY / Repo Gardening checks are stuck due to a github issue.