Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Themes: ssr-ready modules needed for Showcase #3386

Merged
merged 1 commit into from
Feb 22, 2016
Merged

Themes: ssr-ready modules needed for Showcase #3386

merged 1 commit into from
Feb 22, 2016

Conversation

ehg
Copy link
Member

@ehg ehg commented Feb 18, 2016

In #3279 & #3333 we're using a bunch more modules on the server. This means we have to check they're compatible with server-side-rendering, and proceed as per https://github.com/Automattic/wp-calypso/tree/master/docs/server-side-rendering.md

Here we:

  • @ssr-ready what we need for the aforementioned PRs
  • Set viewports on the server to a default of 1024 — contentious!
  • noop certain modules with webpack's NMR, that won't work on the server (yet) 1
  • Ignore any nooped module in the webpack PragmaCheckerPlugin
  • Ignore wp in webpack PragmaCheckerPlugin, as it's a client/server split module
  • Ignore lib/mixins/i18n, as it runs OK now, and we'll be getting to making it isomorphic in the very near future
  • Get rid of non-setState asynchronous calls in componentDidMount
  • Convert Search and SelectDropdown to use saner instance id method.
  • Directly use i18n mixin, instead of relying on it being mixed in via injection (the server hates this)

Components with modified code:

  • HeaderCake
  • Search
  • SelectDropdown
  • mixins/observe-window-resize
  • lib/viewport

To test:

  • Check that all the modified components still work as expected (ARIA included)

1 NMR === NormalModuleReplacementPlugin. We replace the module in the server bundle with something like lodash's noop. We do this because converting every single ssr-incompatible module at once is extremely difficult, so this is a piecemeal approach. It is not a long term solution.

/cc @mcsf @ockham @seear

@ehg ehg added [Feature Group] Appearance & Themes Features related to the appearance of sites. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Server Side Rendering labels Feb 18, 2016
@ehg ehg self-assigned this Feb 18, 2016
@ehg ehg added this to the Themes: Showcase M4-ThemeSheets milestone Feb 18, 2016
blowery
blowery reviewed Feb 18, 2016
View reviewed changes
webpack.config.node.js
@@ -73,7 +73,10 @@ module.exports = {
},
plugins: [
// Require source-map-support at the top, so we get source maps for the bundle
new webpack.BannerPlugin( 'require( "source-map-support" ).install();', { raw: true, entryOnly: false } )
new webpack.BannerPlugin( 'require( "source-map-support" ).install();', { raw: true, entryOnly: false } ),
new webpack.NormalModuleReplacementPlugin( /^analytics$/, 'lodash/utility/noop' ), // Depends on BOM
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we'll have to change these to use the new path for lodash

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Changed.

@ehg ehg force-pushed the update/ssr-ready-modules-for-themes branch from 824e23d to 32d0f2b Compare February 18, 2016 19:07
@ehg ehg mentioned this pull request Feb 19, 2016
Merged
@ehg ehg changed the title Themes ssr-ready modules needed for Showcase Themes: ssr-ready modules needed for Showcase Feb 19, 2016
@ehg ehg mentioned this pull request Feb 19, 2016
Merged
@seear
Copy link
Contributor

seear commented Feb 19, 2016

Set viewports on the server to a default of 1024 — contentious!

If we're going to conjure up a number for now, we must have stats somewhere on the most commonly used widths?

@ehg
Copy link
Member Author

ehg commented Feb 19, 2016

If we're going to conjure up a number for now, we must have stats somewhere on the most commonly used widths?

The problem with setting a single width, is that it could cause weirdness on the initial render. See #3386 (comment)

seear
seear reviewed Feb 19, 2016
View reviewed changes
client/components/search/index.jsx
@@ -255,13 +262,13 @@ module.exports = React.createClass( {
? this._keyListener.bind( this, 'openSearch' )
: null
}
aria-controls={ 'search-component-' + this.id }
aria-controls={ 'search-component-' + this.state.instanceId }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this causing any problems with react tree reconciliation? As far as I remember, it turned out not to be a problem (though I'm not sure why not).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty annoying to test, since I get root replaced errors when testing logged out in #3333 — which I think are obscuring any reconciliation errors.

I remember it causing problems with reconciliation, although my memory is hazy. From what I remember, the id would be different on the server vs the client, as the counter gets reset when the client loads, but increments forever on the server. I'm hoping that statics may help with this, although it's difficult to test.

It could be worth whipping up a /design SSR attempt now, just to see what happens with Search reconc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be worth whipping up a /design SSR attempt now, just to see what happens with Search reconc.

I suggest tackling this later if it becomes a problem.

@seear
Copy link
Contributor

seear commented Feb 19, 2016

From the linked SSR doc:

Must not change component ID during componentWillMount.

Can we add a rationale for this?

mcsf
mcsf reviewed Feb 19, 2016
View reviewed changes
client/components/search/index.jsx
@@ -17,13 +19,16 @@ import { isMobile } from 'lib/viewport';
/**
* Internal variables
*/
var _instance = 1,
SEARCH_DEBOUNCE_MS = 300;
var SEARCH_DEBOUNCE_MS = 300;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hidden tab char :)

@seear
Copy link
Contributor

seear commented Feb 19, 2016

Good to merge, other than my innerWidth query above.

@mcsf
Copy link
Member

mcsf commented Feb 19, 2016

Though touching lots of parts, looks pretty sane code-wise. Haven't properly tested yet.

@ehg
Copy link
Member Author

ehg commented Feb 19, 2016

Added some more fixes in be69f2a & 4c12f92. Could use another quick review :)

@seear
Copy link
Contributor

seear commented Feb 19, 2016

LGTM 👍

@ehg
SSR: Make modules needed for themes SSR-compatible
9d09e92 
- Switch to static and state instance id in Search and SectionNav
- Mark needed modules as `@ssr-ready`
- Use webpack's NormalModuleReplacementPlugin to noop modules that are
  currently non-compatible with SSR
- Use i18n mixin directly, since ReactInjection doesn't work well
  server-side
- Factor window.innerWidth usage into viewport's new getWindowInnerWidth()
@ehg ehg force-pushed the update/ssr-ready-modules-for-themes branch from e7feb1b to 9d09e92 Compare February 19, 2016 20:22
ehg added a commit that referenced this pull request Feb 22, 2016
@ehg
Merge pull request #3386 from Automattic/update/ssr-ready-modules-for…
578bc06 
…-themes

Themes: ssr-ready modules needed for Showcase
@ehg ehg merged commit 578bc06 into master Feb 22, 2016
@ehg ehg deleted the update/ssr-ready-modules-for-themes branch February 22, 2016 11:56
@ehg ehg removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@ehg ehg

Labels
[Feature Group] Appearance & Themes Features related to the appearance of sites. Server Side Rendering
Projects
None yet
Milestone
Themes: Showcase M4-ThemeSheets
Development

Successfully merging this pull request may close these issues.
5 participants
@ehg @seear @mcsf @blowery @mtias