feat(gatsby): use production React for dev-ssr when CI=true

[KyleAMathews]

We don't need React's dev hints & we want SSR to be as fast as possible on CI

[ascorbic]

Could this mean that we miss errors, as we're testing something different from users?

[pieh]

Could this mean that we miss errors, as we're testing something different from users?

Probably will not miss errors (tho wording might be different), but warnings on CI wouldn't show, but it doesn't seem like we are testing for those right now.

If there are worries about using generic check testing if we are in CI - maybe this should not care about CI env and instead should be setting for DEV_SSR (wether to use prod or dev react / react-dom/server)? GATSBY_DEV_SSR_USE_PROD_REACT (or something like that)?

[KyleAMathews]

@ascorbic this isn't for testing — it's for people running preview servers w/ gatsby develop — there we're assuming people aren't going to be looking at SSR warnings anyways and we want the SSR to be as fast as possible. The dev versions of React are for development which is what we run on people's actual machines.

[ascorbic]

We could check if we're running through Cypress, which is what I'm concerned about.

[pieh]

We could check if we're running through Cypress, which is what I'm concerned about.

Just to be clear - this is for SSR not browser bundle. At the compile time we also have no ability to know if we will run through cypress. We can add some toggle to disable using Prod react for dev SSR for our tests, but so far we don't have any tests for dev ssr that would actually check for warnings (thought we should add some at some point)

[pieh]

Even when using externals I found cases when dev version is used - it's when other externals import webpack (

gatsby/packages/gatsby/src/utils/webpack.config.js

Lines 638 to 655 in abdb8d6

	const externalList = [
	`@reach/router/lib/history`,
	`@reach/router`,
	`common-tags`,
	`crypto`,
	`debug`,
	`fs`,
	`https`,
	`http`,
	`lodash`,
	`path`,
	`semver`,
	/^lodash\//,
	`zlib`,
	]
	// Packages we want to externalize because meant to be user-provided
	const userExternalList = [`react-helmet`, `react`, /^react-dom\//]

)

In particular @reach/router caused this (I added console.trace() to check), but react-helmet would be another case for it I assume.

Unless we make those not externals anymore I don't see a way to address it without hooking into node.js module loading and making overwrites there too (but that seem messy ...).

In any case - it doesn't seem to cause problems in my testing. The fact remains that it will load react module twice (dev and prod) and it will use dev react for some thing and prod for most of things

[pieh]

To add to above - funnily enough - react-dom also import react, so react-dom being externals is one of things that cause to use dev version of react :(

[pieh]

Let's get this in as-is. Main benefit is from using prod version of react-dom/server renderToString - we possibly can squeeze more - but this is good enough for now

[vladar]

Published in [email protected]