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

fix(DEV_SSR): set NODE_ENV to production for render-html worker process instead of pointing externals to production react #29620

Merged
merged 3 commits into from
Feb 22, 2021
Merged

fix(DEV_SSR): set NODE_ENV to production for render-html worker process instead of pointing externals to production react #29620

merged 3 commits into from
Feb 22, 2021

Conversation

KyleAMathews
Copy link
Contributor

@KyleAMathews KyleAMathews commented Feb 20, 2021 •
edited
Loading

Fix #28138 (comment)

Set NODE_ENV to production in the child worker that does the SSR in dev.

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Feb 20, 2021
@wardpeet
Copy link
Contributor

wardpeet commented Feb 20, 2021 •
edited
Loading

You should try with the @next version and if it doesn't work add an alias to:
https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/src/utils/webpack.config.js#L422-L423

@KyleAMathews
Copy link
Contributor Author

@wardpeet which branch do I make changes to v2 on? Just releases/2.32? Will test against v3 as well.

@wardpeet
Copy link
Contributor

Yes :)

@pieh pieh self-assigned this Feb 21, 2021
@pieh pieh added topic: ssr and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Feb 21, 2021
@pieh
Copy link
Contributor

pieh commented Feb 22, 2021

Does it trully work? react-dom imports react ( https://unpkg.com/browse/react-dom@17.0.1/cjs/react-dom-server.node.development.js ), so that can result in multiple react as well (one bundled from one we remove externals here and one unbundled (imported by external react-dom)

@KyleAMathews
Copy link
Contributor Author

@pieh quite possibly not :-D

I thought of a much simpler approach — we render the SSR in a worker so just set the NODE_ENV to production there. So no more hackery. Seems to work perfectly now in my testing.

@KyleAMathews KyleAMathews mentioned this pull request Feb 22, 2021
Merged
KyleAMathews added a commit that referenced this pull request Feb 22, 2021
@KyleAMathews
fix(gatsby): more reliable way to use prod versions of react/react-dom
6eb7d38 
back port of #29620 to v2

Fix #28138 (comment)

Set `NODE_ENV` to `production` in the child worker that does the SSR in dev.
pieh
pieh approved these changes Feb 22, 2021
View reviewed changes
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

Nice!

@pieh
Copy link
Contributor

pieh commented Feb 22, 2021

Just for paper trail - I used https://github.com/pieh/dev-ssr-multi-react-hooks as reproduction for issue this is solving (in case someone git blame some changes back to this PR)

@pieh pieh changed the title fix(gatsby): fix DEV_SSR by only setting react-dom/server to use prod version fix(DEV_SSR): set NODE_ENV to production for render-html worker process instead of pointing externals to production react Feb 22, 2021
@pieh pieh merged commit d8772ec into master Feb 22, 2021
@pieh pieh deleted the fix-ssr branch February 22, 2021 21:41
KyleAMathews added a commit that referenced this pull request Feb 23, 2021
@KyleAMathews @wardpeet @ascorbic
fix(gatsby): more reliable way to use prod versions of react/react-dom (
01d07b3 
#29683)

back port of #29620 to v2

Fix #28138 (comment)

Set `NODE_ENV` to `production` in the child worker that does the SSR in dev.

Co-authored-by: Ward Peeters <ward@coding-tech.com>
Co-authored-by: Matt Kane <matt@gatsbyjs.com>
@LekoArts LekoArts added the topic: DX Developer Experience (e.g. Fast Refresh, i18n, SSR, page creation, starters) label May 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pieh pieh pieh approved these changes

Assignees

@pieh pieh

Labels
topic: DX Developer Experience (e.g. Fast Refresh, i18n, SSR, page creation, starters)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@KyleAMathews @wardpeet @pieh @LekoArts