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.

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

Server: Use production react in the server bundle for prod #10693

Merged
merged 2 commits into from
Jan 18, 2017

Conversation

blowery
Copy link
Contributor

@blowery blowery commented Jan 17, 2017

See https://twitter.com/eldh/status/821257104694579200

Currently we're running the debug build of react in our server process. This should switch it to be the production build, which should cut down render times a fair bit.

@matticbot
Copy link
Contributor

@blowery blowery requested review from ockham and ehg January 17, 2017 13:10
@blowery blowery added Server Side Rendering [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 17, 2017
@blowery blowery force-pushed the update/server/use-production-react branch from 65448a6 to 3e8b929 Compare January 17, 2017 13:20
@ehg
Copy link
Member

ehg commented Jan 17, 2017

Currently we're running the debug build of react in our server process. This should switch it to be the production build, which should cut down render times a fair bit.

Hmm, I think maybe this should go in the Dockerfile? e.g. ENV NODE_ENV=production. Using DefinePlugin seems a bit weird, since we'll be messing with node's process object.

@blowery
Copy link
Contributor Author

blowery commented Jan 17, 2017

Hmm, I think maybe this should go in the Dockerfile? e.g. ENV NODE_ENV=production. Using DefinePlugin seems a bit weird, since we'll be messing with node's process object.

I had the plugin in the wrong place at first. It should be a webpack plugin, not a babel plugin.

I also turned on minification so the runtime checks for NODE_ENV don't even happen now in prod.

@ehg
Copy link
Member

ehg commented Jan 17, 2017

I had the plugin in the wrong place at first. It should be a webpack plugin, not a babel plugin.

I only saw the webpack plugin, so stand by my statement :) I think we're likely missing a production NODE_ENV var here, when building: https://github.com/Automattic/wp-calypso/blob/master/Dockerfile#L42 — not sure what systems pass to the docker build from the build script, though.

@blowery
Copy link
Contributor Author

blowery commented Jan 17, 2017

I only saw the webpack plugin, so stand by my statement :)

:) The important thing here is to run the minified version of React, not to run with NODE_ENV=production. Minification strips out a lot of code that forces V8 to deoptimize important functions, mostly because the removed code contains try / catch blocks. So it removes things like:

function doAThing() {
  if ( process.env.NODE_ENV !== 'production' ) {
    try {
      validationThatMightThrow();
    } catch( e ) {
      console.error( e );
    }
  }

  doTheActualWork();
}

By removing the chunk with the try block, V8 can optimize the function. With it in place, it will never optimize it.

There's also other things that envify'ing react fixes that are not disabled simply by setting NODE_ENV to production.

See https://twitter.com/dan_abramov/status/592691315374170113

@blowery
Copy link
Contributor Author

blowery commented Jan 17, 2017

Also raxjs/server-side-rendering-comparison#5

This is because process.env is not a simple JavaScript object, and it is surprisingly expensive to use it. Even when running in production mode, just the calls to check the procces.env.NODE_ENV can easily take 30% of rendering time in React.

@ehg
Copy link
Member

ehg commented Jan 17, 2017

The important thing here is to run the minified version of React, not to run with NODE_ENV=production. Minification strips out a lot of code that forces V8 to deoptimize important functions, mostly because the removed code contains try / catch blocks.

Sure, but we can set the env at build time without DefinePlugin right? I totally get why we want to minify.

I guess I'm worried about other env vars we depend on being missed, e.g. CALYPSO_ENV for one. The runtime/build time difference in env could trip us up here :]

This is because process.env is not a simple JavaScript object, and it is surprisingly expensive to use it. Even when running in production mode, just the calls to check the procces.env.NODE_ENV can easily take 30% of rendering time in React.

Ah, hmm, good find. This is interesting. nodejs/node#3104 has some more details.

@blowery
Copy link
Contributor Author

blowery commented Jan 17, 2017

Sure, but we can set the env at build time without DefinePlugin right?

How would we do that?

@blowery
Copy link
Contributor Author

blowery commented Jan 17, 2017

Sorry, that was too terse. Yay phones.

Setting the env at build time won't have any affect. I assume you mean something like NODE_ENV=production webpack ...? That won't help because the build doesn't run the code being built, it just parses it, modifies it, and spits it back out. DefinePlugin works by actually replacing the code in the source with a hard-coded value. So

if ( process.env.NODE_ENV === 'production' )

becomes

if ( 'production' === 'production' )

which is then sent to Uglify, which then removes the block in question because it can guarantee that the condition is always false. Setting the env var doesn't get us that.

Now, if we have similar checks for environment vars in our code, we should add those to DefinePlugin assuming they're constant at build time. I'm not positive, but pretty sure, that given how we use DefinePlugin, we're not replacing checks to other env vars.

@blowery blowery force-pushed the update/server/use-production-react branch from a8b0db9 to 7659d22 Compare January 17, 2017 16:15
@blowery
Copy link
Contributor Author

blowery commented Jan 17, 2017

@ehg found a better, simpler way to do this: just remap the react and react-with-addons externals to the minified version shipped with react. Done in 7659d22

@blowery blowery force-pushed the update/server/use-production-react branch from 7659d22 to 49402a5 Compare January 17, 2017 16:17
@ehg
Copy link
Member

ehg commented Jan 17, 2017

just remap the react and react-with-addons externals to the minified version shipped with react.

This was necessary as we treat node_modules as external deps: https://github.com/Automattic/wp-calypso/blob/master/webpack.config.node.js#L27

So minifying/DefinePlugin would likely have little effect here.

@blowery
Copy link
Contributor Author

blowery commented Jan 17, 2017

As a possible follow on to this, we should check and see if other modules we're using server side take advantage of the same pattern (redux? redux-react?). Anything that's gating on process.env.NODE_ENV is a candidate.

@blowery
Copy link
Contributor Author

blowery commented Jan 17, 2017

Confirmed this works in a docker container

@blowery
Copy link
Contributor Author

blowery commented Jan 17, 2017

As a possible follow on to this, we should check and see if other modules we're using server side take advantage of the same pattern (redux? redux-react?).

Looks like redux does use it to check for unexpected keys

https://github.com/reactjs/redux/blob/master/src/combineReducers.js#L139

@blowery
Copy link
Contributor Author

blowery commented Jan 17, 2017

Reworked to only remap to minified react (and now redux) in prod environments.

@blowery
Copy link
Contributor Author

blowery commented Jan 17, 2017

Hrm. /design isn't loading completely on my local docker instance running wpcalypso with an incognito instance in Chrome. Works fine on https://wpcalypso.wordpress.com. Perhaps an https issue?

SSR output looks the same. Guessing it's something with the local docker setup.

@blowery
Copy link
Contributor Author

blowery commented Jan 17, 2017

Confirmed http://wpcalypso.wordpress.com/theme/mood works in a local docker env

@ehg
Copy link
Member

ehg commented Jan 17, 2017

Hrm. /design isn't loading completely on my local docker instance running wpcalypso with an incognito instance in Chrome. Works fine on https://wpcalypso.wordpress.com.

I get this too, same on master, there are no XHR requests made.

FWIW only the masterbar is SSRed on logged-out /design.

Perhaps an https issue?

Perhaps. It would be great if something like #9276 could allow us to mirror the different envs as closely as possible, including a reverse nginx proxy with self-signed SSL cert to avoid what I think are HSTS issues with testing http://wordpress.com locally?

Copy link
Member

@ehg ehg left a comment

Choose a reason for hiding this comment

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

Anyway, looks good to go to me :)

@blowery blowery merged commit a01a0cb into master Jan 18, 2017
@blowery blowery deleted the update/server/use-production-react branch January 18, 2017 14:04
@blowery blowery removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants