Themes: Use `details` middleware for server-side rendering #3562

ockham · 2016-02-25T14:20:33Z

In the /themes route in server/pages, use my-sites/themes to create a theme sheet element tree in context.layout, and renderToString() that.

Adapt express's req context to be able to use it with middleware that otherwise operates on page's context. props @mcsf
Add a package.json that sets different entry points on the server and in the browser (not everything required by my-sites/themes/index.js is SSR-ready yet, so we need a pruned version for the server).
Move theme details specific async data fetching to a separate middleware in my-sites/themes/controller/index.node.js.
Move generic renderToString caching to server/render.
Cache Helmet data, so title etc are properly set.

To test:

Check that everything still works as before.
Try logged-out /theme/mood, with JS disabled. Note that the title is now set to 'themeName Theme — WordPress.com' (instead of the generic 'WordPress.com', as on master).
Note that logged-in /design and /themes/something routes server-render the logged-out layout (masterbar!) before it's being replaced by the logged-in one on the client. While we want to fix this in the long run, it is no behavioral change from master.

Note:

There are still reconciliation errors, but there are also some in master. Opting to fix them later, since this PR is big enough already. (Possible cuplrit: Apparently, react-helmet inserts a noscript tag. server rendering returns <noscript></noscript> where <Helmet /> is implemented nfl/react-helmet#81)
SEO is still broken for SSR'd /design. Also opting to fix later.

/cc @seear @ehg @mcsf

Follow-ups: #3302, #3774, #3795, #3425

I'd say this closes #185 :-)

ockham · 2016-02-25T22:40:27Z

Getting

Error: Cannot find module 'indexof'
    at Function.Module._resolveFilename (module.js:325:15)
    at Function.Module._load (module.js:276:25)
    at Module.require (module.js:353:17)
    at require (internal/module.js:12:17)
    at Object.<anonymous> (wp-calypso/node_modules/component-classes/index.js:5:13)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Module.require (module.js:353:17)
Makefile:96: recipe for target 'run' failed
make: *** [run] Error 1

@mcsf @ehg @seear Any experience from past component woos how to fix that?

ehg · 2016-02-26T10:53:41Z

Any experience from past component woos how to fix that?

NMR it, and seek an upstream fix :)

ockham · 2016-02-26T17:58:35Z

Been there, it seems: 8447-gh-calypso-pre-oss :-/

ockham · 2016-02-26T18:05:49Z

Hmm, just adding new webpack.NormalModuleReplacementPlugin( /^indexof$/, 'component-indexof' ) doesn't do the trick (nor does lodash/noop) -- probably because this is happening somewhere down in node_modules/, which we're not bundling through webpack?

ockham · 2016-02-26T18:24:39Z

seek an upstream fix :)

component/classes#27

ehg · 2016-02-26T19:24:14Z

Right, so the way we're trying to avoid loading non-iso-compatible controllers is suspect.

import { loggedOut, details, makeLoggedOutLayout } from './controller'; will result in attempts to resolve and load all dependencies in the controller.

What I imagined was having separate node/web controllers, so this wouldn't happen. Maybe put all SSR-compatible controllers/middleware in controllers/index.node.js, and have controllers/index.web.js specify the web-only stuff, and also pull in controllers/index.node.js, so we're DRY.

FWIW, I think we can solve the component.js issues using webpack's resolve.alias: https://webpack.github.io/docs/configuration.html#resolve-alias — but that's not going to help us much at this point in time.

ockham · 2016-02-29T00:59:48Z

server/isomorphic-routing/index.js

+		Object.assign( context, render( context.layout ) );
+	}
+	context.res.render( 'index.jade', context );
+}


Should move this function to server/render, I think.

ockham · 2016-03-03T21:17:33Z

Rebasing this on master now that #3279 is in turns out to be a major PITA. I'm afraid we'll have to untangle some code in server/pages with a separate PR before this is feasible...

ockham · 2016-03-08T16:13:58Z

server/isomorphic-routing/index.js

+	compose( ...liftedmiddlewares )();
+}
+
+function compose( ...functions ) {


Can we use lodash's flowRight instead?

(For the record, flowRight wasn't fit for a drop-in replacement. It was agreed that having this local implementation is fine.)

mcsf · 2016-03-08T16:52:22Z

client/my-sites/themes/controller/index.node.js

+		<ThemeDetailsComponent id={ themeSlug } section={ contentSection } >
+			<ThemeSheetComponent />
+		</ThemeDetailsComponent>
+	);


We're redeclaring a component on each middleware call here.

seear · 2016-03-09T15:03:07Z

Reviewed/tested as a team

Add a `package.json` that sets different entry points on the server and in the browser (not everything required by `my-sites/themes/index.js` is SSR-ready yet, so we need a pruned version for the server).

@mcsf

In the `/design` and `/themes` routes in `server/pages`, use `my-sites/themes` to create element trees in `context.layout`, and `renderToString()` that. * Adapt `express`'s `req` context to be able to use it with middleware that otherwise operates on `page`'s `context` (props @mcsf). * Add docs

To avoid confusion with `context.layout` holding a Layout *element*.

…ddleware-server-side Themes: Use `details` middleware for server-side rendering

ockham added [Feature Group] Appearance & Themes Features related to the appearance of sites. [Status] In Progress Server Side Rendering labels Feb 25, 2016

ockham self-assigned this Feb 25, 2016

ockham added this to the Themes: Showcase M4-ThemeSheets milestone Feb 25, 2016

This was referenced Feb 25, 2016

Try/isomorphic routing #3302

Closed

Themes: Render logged-out layout last #3516

Merged

ockham force-pushed the update/themes-use-details-middleware-server-side branch 4 times, most recently from 5091c86 to f71f418 Compare February 25, 2016 18:54

ockham reviewed Feb 29, 2016
View reviewed changes

ockham force-pushed the update/themes-use-details-middleware-server-side branch from 7d15e15 to 965fd4f Compare March 2, 2016 14:55

This was referenced Mar 3, 2016

Themes: Bootstrap theme-details store for theme sheets #3279

Merged

Themes SSR: Rely on Redux store for caching themeDetails API object #3774

Closed

ockham force-pushed the update/themes-use-details-middleware-server-side branch 4 times, most recently from ef15ccd to d2af217 Compare March 5, 2016 22:01

ockham added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Mar 6, 2016

ockham force-pushed the update/themes-use-details-middleware-server-side branch from 21e597f to 791bcde Compare March 6, 2016 17:33

ockham reviewed Mar 8, 2016
View reviewed changes

mcsf reviewed Mar 8, 2016
View reviewed changes

ockham added 17 commits March 9, 2016 15:04

Themes: Add package.json switch

9ea8363

Add a `package.json` that sets different entry points on the server and in the browser (not everything required by `my-sites/themes/index.js` is SSR-ready yet, so we need a pruned version for the server).

index.jade, server/render: Rename layout to renderedLayout

9b554eb

To avoid confusion with `context.layout` holding a Layout *element*.

server/render: Actually assign helmet data to context

4d1ae84

layout/logged-out: Fix a propType spec

696a342

Move makeLoggedOutLayout() to client/controller

23d1152

Update SSR docs

a1e3887

Move async data fetching to themes/controller

7e5e20b

layout/logged-out: Cosmetic changes

59a24e6

server/render: Cache Helmet data

5c3a632

ThemesHead: Replace isSheet by title prop

700ca07

Themes: Set proper page title

0647615

themes/controller: Use makeElement()

0ca1511

Dedupe, by makin receiveThemeDetails() an action

eb89464

client/boot: Remove obsolete code

589e0af

Dispatch setSection() for logged-out /design on the server

b7d79e7

Inline some functions to get rid of misleading function names

0c6abfa

ockham force-pushed the update/themes-use-details-middleware-server-side branch from d72b9fd to 0c6abfa Compare March 9, 2016 15:05

ockham added a commit that referenced this pull request Mar 9, 2016

Merge pull request #3562 from Automattic/update/themes-use-details-mi…

f983440

…ddleware-server-side Themes: Use `details` middleware for server-side rendering

ockham merged commit f983440 into master Mar 9, 2016

ockham deleted the update/themes-use-details-middleware-server-side branch March 9, 2016 15:12

nylen removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 12, 2016

This was referenced Mar 14, 2016

Framework: add "section" declarative configuration to the state tree #3913

Merged

Framework: Bootstrap when isorouting #4081

Closed

tyxla mentioned this pull request Dec 19, 2016

Framework: Remove path-parser dependency #10154

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Themes: Use `details` middleware for server-side rendering #3562

Themes: Use `details` middleware for server-side rendering #3562

ockham commented Feb 25, 2016

ockham commented Feb 25, 2016

ehg commented Feb 26, 2016

ockham commented Feb 26, 2016

ockham commented Feb 26, 2016

ockham commented Feb 26, 2016

ehg commented Feb 26, 2016

ockham Feb 29, 2016

ockham commented Mar 3, 2016

ockham Mar 8, 2016

mcsf Mar 13, 2016

mcsf Mar 8, 2016

seear commented Mar 9, 2016

Themes: Use details middleware for server-side rendering #3562

Themes: Use details middleware for server-side rendering #3562

Conversation

ockham commented Feb 25, 2016

ockham commented Feb 25, 2016

ehg commented Feb 26, 2016

ockham commented Feb 26, 2016

ockham commented Feb 26, 2016

ockham commented Feb 26, 2016

ehg commented Feb 26, 2016

ockham Feb 29, 2016

Choose a reason for hiding this comment

ockham commented Mar 3, 2016

ockham Mar 8, 2016

Choose a reason for hiding this comment

mcsf Mar 13, 2016

Choose a reason for hiding this comment

mcsf Mar 8, 2016

Choose a reason for hiding this comment

seear commented Mar 9, 2016

Themes: Use `details` middleware for server-side rendering #3562

Themes: Use `details` middleware for server-side rendering #3562