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

seear · 2016-02-12T15:44:29Z

Add the theme-details data component to the layout
Fetch and cache theme details server-side and bootstrap to the client

To Test
Go to http://calypso.localhost:3000/themes/{theme} both logged-in and logged-out

seear · 2016-02-16T20:18:39Z

webpack.config.js

@@ -97,7 +97,7 @@ jsLoader = {
 };

 if ( CALYPSO_ENV === 'development' ) {
-	webpackConfig.plugins.push( new PragmaCheckPlugin() );
+//	webpackConfig.plugins.push( new PragmaCheckPlugin() );


Problem: state/themes/actions.js is replaced with noop in webpack.config.node.js but the pragma checker doesn't know this, so we still get the not /** @ssr-ready **/ error. @ehg: any idea how we should handle this?

Hmm, we can add analytics to the ignore array, but I think state/themes/actions.js should be ssr-ready from the start. Will take a look.

Pushed acc169f

ehg · 2016-02-18T19:15:44Z

server/pages/index.js

+			if ( config.isEnabled( 'server-side-rendering' ) ) {
+				let themeData = themeDetails.get( req.params.theme_slug );
+				if ( ! themeData ) {
+					wpcom.undocumented().themeDetails( req.params.theme_slug, ( error, data ) => {


I wonder if we could get away without having to use memcache by:

Checking the published date of the theme details response

If it's greater than the one in the cache, renderToString using the new data.

If it's not, just serve the cached renderToString markup

We could also avoid blocking the request by always serving the data from the cache, and fire off API calls that will invalidate/update the cache when necessary.

I like that idea :-)

We could also avoid blocking the request by always serving the data from the cache, and fire off API calls that will invalidate/update the cache when necessary.

This PR was actually working like that a few commits back and can easily be tweaked to do that again. I wasn't quite sure which way was best. It strikes me now that never rendering async is less complex overall.

I wonder if we could get away without having to use memcache by...

Yeah, was thinking about this, or maybe even using the api response header, which may let us leverage some existing libraries to handle it?

Once we receive new api data, how do we clear the renderToString cache?

Yeah, was thinking about this, or maybe even using the api response header, which may let us leverage some existing libraries to handle it?

Yep. I wonder whether we sent ETags or what the Expires header is like.

Once we receive new api data, how do we clear the renderToString cache?

You mean how do we know what key to clear? Maybe we key the rts cache using something like dataTimestamp+lang?

I wonder whether we sent ETags or what the Expires header is like.

Expires and cache control headers are explicitly set to not cache, which makes sense. I think comparing the publish/update date is probably fine for now.

RE #3279 (comment) - implemented a basic version of this in b560f0a

ehg · 2016-02-18T19:38:22Z

Rebased, not using makeElement() for now.

ehg · 2016-02-19T15:52:01Z

Changed to 'In Progress', as this should be based off #3386, and not merged until that lands. Also, we need more discussion around #3279 (comment)

ehg · 2016-02-19T16:00:00Z

this should be based off #3386

Done.

ockham · 2016-02-19T19:39:56Z

server/pages/index.js

+					themeName: theme.name,
+					themeAuthor: theme.author,
+					themeScreenshot: theme.screenshot,
+				} );


Could this call go somewhere deeper in the component hierarchy, e.g. inside ThemesListFetcher (to provide some initial state there, also on the client side)? Thinking about how to keep this part of server/pages as generic as possible for isomorphic routing.

Actually, renderThemeSheet looks like something we could keep as middleware in my-sites/themes/controller: it basically writes to context and dispatches stuff via store, which should be available via context.

mcsf · 2016-02-22T17:15:06Z

server/pages/index.js

-	setSection = require( 'state/ui/actions' ).setSection;
+	setSection = require( 'state/ui/actions' ).setSection,
+	wpcom = require( 'lib/wp' ),
+	ActionTypes = require( 'state/themes/action-types' );


Should be ThemesActionTypes

ehg · 2016-02-23T14:18:57Z

Rebased.

ehg · 2016-02-23T17:25:16Z

Added dynamic data to content sections and fixed price. Next up: make code not suck.

seear · 2016-02-24T11:30:24Z

rebased after #3464

seear · 2016-02-24T14:13:44Z

client/my-sites/themes/controller.js

+	//TODO: use makeElement()
+	context.primary = (
+		<ReduxProvider store={ context.store } >
+			<Head title={ props.title } isSheet>


We're still using the hardcoded Pineapple Fifteen title, but I think that should be hooked up in a separate PR

wp-calypso/client/my-sites/themes/head.jsx

Line 15 in 339c6bb

title={ isSheet ? 'Pineapple Fifteen Theme — WordPress.com' : get( 'title', tier ) }

Yeah, I think we should leave this until after routing. It's possibly being implemented in one of those PRs.

Maybe this stuff, with some modifications? -> https://github.com/Automattic/wp-calypso/pull/3425/files#diff-85f90b61d9c36634c5464a3180470726R70

seear · 2016-02-25T12:53:40Z

server/render/index.js

-		bumpStat( 'calypso-ssr', 'loggedout-design-cache-miss' );
-	}
-
+export function render( element, path, initialState ) {


Maybe better to have a key param instead of path and initialState, allowing caller to decide appropriate key?

Yeah, also I was thinking the API could be something like render.set( key, element ) and render.get( key )

Yeah, also I was thinking the API could be something like render.set( key, element ) and render.get( key )

I think better to put a comment before the async renderThemeSheet() than expose the caching from the render module.

ehg · 2016-02-26T12:22:30Z

server/pages/index.js

+			function updateRenderCache( themeSlug ) {
+				wpcom.undocumented().themeDetails( themeSlug, ( error, data ) => {
+					if ( error ) {
+						console.log( `Error fetching theme ${ themeSlug } details: `, error.message || error );


Can we use debug for these console.logs?

ehg · 2016-02-26T12:34:08Z

👍 after squash. Let's get this in and iterate.

Wrap theme sheets with the theme-details component to use real data, and fetch and cache data server-side for initial render. Server-side data caching: - if the theme's details aren't in the themeDetails cache, don't SSR & fire and forget a theme details XHR, which will renderToString when it has data for the theme. - if the theme is in the cache, then we'll have markup for it, so render that doing it this way means we don't have to block the request, but we lose SSR for cache misses. i'm unsure of whether blocking on the response will be a problem (need timing stats), if it is, memcache or a CDN would be preferable.

Themes: Bootstrap theme-details store for theme sheets

ockham · 2016-03-03T21:13:54Z

server/pages/index.js

+			if ( config.isEnabled( 'server-side-rendering' ) ) {
+				const theme = themeDetails.get( req.params.theme_slug );
+				if ( theme ) {
+					Object.assign( context, renderThemeSheet( theme ) );


Is it possible that this code path ends up renderingToString synchronously via renderThemeSheet() and render(), i.e. blocking the request?

ockham · 2016-03-03T21:42:22Z

I find the two different caches and how they interact rather confusing, hard to reconcile with #3562, and possibly error-prone. If we want to cache the markup based on date_updated, can't we just make it part of the markup cache key here -- possibly by just exposing it as part of the themeDetails object we dispatch here, so it becomes part of the initialReduxState, which is then used for the key?

If we just want to cache the theme details data for their own sake, we should do that inside our Redux reducer -- it's made for storing data for different theme IDs already, so we're kind of re-creating that logic here.

ockham · 2016-03-03T22:26:09Z

server/pages/index.js

+						debug( 'caching', themeSlug );
+						themeDetails.set( themeSlug, data );
+						// update the render cache
+						renderThemeSheet( data );


Why don't we also render index.jade here?

ockham · 2016-03-04T00:57:42Z

Filed #3774 based on my comments.

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

seear self-assigned this Feb 12, 2016

seear added this to the Themes: Showcase M4-ThemeSheets milestone Feb 12, 2016

seear mentioned this pull request Feb 15, 2016

Themes: Add themes detail page #3053

Merged

3 tasks

seear force-pushed the add/themes-sheet-data branch 2 times, most recently from 8595df2 to 8b809d7 Compare February 15, 2016 17:56

ehg mentioned this pull request Feb 15, 2016

Themes: View a theme's title and author #2875

Closed

6 tasks

seear 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 Feb 16, 2016

seear reviewed Feb 16, 2016
View reviewed changes

This was referenced Feb 17, 2016

Themes: See a screenshot in the sheet view #2876

Closed

Themes: ssr-ready modules needed for Showcase #3386

Merged

ehg reviewed Feb 18, 2016
View reviewed changes

ehg force-pushed the add/themes-sheet-data branch from 1818d64 to 2c6f0f0 Compare February 18, 2016 19:37

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

ehg force-pushed the add/themes-sheet-data branch from 0c9eccf to cf073b9 Compare February 19, 2016 15:59

ockham reviewed Feb 19, 2016
View reviewed changes

seear force-pushed the add/themes-sheet-data branch from 4e37c27 to aa0a25c Compare February 22, 2016 12:55

seear mentioned this pull request Feb 22, 2016

Themes: Handle SERVER_DESERIALIZE actions in reducers #3464

Merged

mcsf reviewed Feb 22, 2016
View reviewed changes

ockham mentioned this pull request Feb 23, 2016

Themes: Record pageView() on Details page #3420

Closed

3 tasks

ehg force-pushed the add/themes-sheet-data branch from 8c9b414 to f8ecf9c Compare February 23, 2016 14:18

seear force-pushed the add/themes-sheet-data branch from 4514c0c to e128fc0 Compare February 24, 2016 11:23

seear reviewed Feb 24, 2016
View reviewed changes

seear force-pushed the add/themes-sheet-data branch from b622acf to 1d17c23 Compare February 25, 2016 12:39

seear reviewed Feb 25, 2016
View reviewed changes

seear 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 Feb 25, 2016

seear force-pushed the add/themes-sheet-data branch from eac589a to e187341 Compare February 25, 2016 22:15

ehg reviewed Feb 26, 2016
View reviewed changes

seear force-pushed the add/themes-sheet-data branch from 361e87c to fb146d1 Compare February 26, 2016 12:58

seear mentioned this pull request Feb 26, 2016

Themes: Handle SERVER_DESERIALIZE action for current-theme #3594

Merged

seear force-pushed the add/themes-sheet-data branch from fb146d1 to f69e60c Compare February 26, 2016 16:26

seear force-pushed the add/themes-sheet-data branch from f69e60c to b2cc08d Compare February 29, 2016 11:12

seear added a commit that referenced this pull request Feb 29, 2016

Merge pull request #3279 from Automattic/add/themes-sheet-data

09a7aad

Themes: Bootstrap theme-details store for theme sheets

seear merged commit 09a7aad into master Feb 29, 2016

seear deleted the add/themes-sheet-data branch February 29, 2016 11:24

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

ockham reviewed Mar 3, 2016
View reviewed changes

ockham mentioned this pull request Mar 3, 2016

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

Merged

ockham reviewed Mar 3, 2016
View reviewed changes

ockham mentioned this pull request Mar 4, 2016

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

seear commented Feb 12, 2016

seear Feb 16, 2016

ehg Feb 17, 2016

ehg Feb 17, 2016

ehg Feb 18, 2016

ockham Feb 18, 2016

seear Feb 19, 2016

seear Feb 19, 2016

ehg Feb 19, 2016

ehg Feb 19, 2016

ehg Feb 23, 2016

ehg commented Feb 18, 2016

ehg commented Feb 19, 2016

ehg commented Feb 19, 2016

ockham Feb 19, 2016

mcsf Feb 22, 2016

mcsf Feb 22, 2016

ehg commented Feb 23, 2016

ehg commented Feb 23, 2016

seear commented Feb 24, 2016

seear Feb 24, 2016

ehg Feb 24, 2016

ockham Feb 25, 2016

seear Feb 25, 2016

ehg Feb 25, 2016

seear Feb 25, 2016

ehg Feb 26, 2016

ehg commented Feb 26, 2016

ockham Mar 3, 2016

ockham commented Mar 3, 2016

ockham Mar 3, 2016

ockham commented Mar 4, 2016

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

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

Conversation

seear commented Feb 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehg commented Feb 18, 2016

ehg commented Feb 19, 2016

ehg commented Feb 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehg commented Feb 23, 2016

ehg commented Feb 23, 2016

seear commented Feb 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehg commented Feb 26, 2016

Choose a reason for hiding this comment

ockham commented Mar 3, 2016

Choose a reason for hiding this comment

ockham commented Mar 4, 2016