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

[Bug]: react-router-serve requires cross-env NODE_ENV=production #12078

Closed
jrestall opened this issue Oct 6, 2024 · 7 comments · Fixed by #12578
Closed

[Bug]: react-router-serve requires cross-env NODE_ENV=production #12078

jrestall opened this issue Oct 6, 2024 · 7 comments · Fixed by #12578
Labels
awaiting release This issue have been fixed and will be released soon bug

Comments

@jrestall
Copy link
Contributor

jrestall commented Oct 6, 2024

What version of React Router are you using?

7.0.0-pre.0

Steps to Reproduce

git clone https://github.com/jacobparis/underkill-stack.git
pnpm install
pnpm build
edit package.json to remove NODE_ENV=production from the "start" script.
pnpm start

HTTP 500: Internal Server Error -> "Unexpected Server Error"

The userland fix for the error is to always specify NODE_ENV=production in the package.json start script.

- "start": "react-router-serve ./build/server/index.js"
+ "start": "cross-env NODE_ENV=production react-router-serve ./build/server/index.js"

This could be because whilst react-router-serve tries to default to NODE_ENV=production, react is loaded earlier than when that code runs and loads the development version of react in the absence of a NODE_ENV env variable. Then later the app server build is dynamically imported and subsequently imports react-dom, but now NODE_ENV has been set to production by react-router-serve, so there is a mismatch in the loaded react libraries with a mix of development and production.

process.env.NODE_ENV = process.env.NODE_ENV ?? "production";

The import chain that results in the react development version being loaded is:

node_modules/@react-router/serve/dist/cli.js
-> @react-router/node
-> node_modules/@react-router/node/dist/index.js
-> node_modules/@react-router/node/dist/sessions/fileStorage.js
-> node_modules/react-router/dist/main.js
-> node_modules/react/index.js
// node_modules/react/index.js
console.log(process.env.NODE_ENV); // -> undefined
if (process.env.NODE_ENV === 'production') {
  module.exports = require('./cjs/react.production.js');
} else {
  module.exports = require('./cjs/react.development.js');
}
// node_modules/react-dom/index.js
console.log(process.env.NODE_ENV); // -> production
if (process.env.NODE_ENV === 'production') {
  module.exports = require('./cjs/react.production.js');
} else {
  module.exports = require('./cjs/react.development.js');
}

So should we always pass a NODE_ENV in the start script or is this an issue?

System Info

Node: v21.1.0
OS: Mac

Expected Behavior

Site returns 200 response.

Actual Behavior

[react-router-serve] http://localhost:3000 (http://192.168.20.16:3000)
TypeError: dispatcher.getOwner is not a function
    at getOwner (/Users/jrestall/Dev/underkill-stack/node_modules/.pnpm/[email protected]/node_modules/react/cjs/react.development.js:412:54)
    at Module.createElement (/Users/jrestall/Dev/underkill-stack/node_modules/.pnpm/[email protected]/node_modules/react/cjs/react.development.js:1331:9)
    at mapRouteProperties (file:///Users/jrestall/Dev/underkill-stack/node_modules/.pnpm/[email protected][email protected][email protected]_bnl2udvc6mytkjbq3r7oxas3x4/node_modules/lib/components.tsx:93:22)
    at map (file:///Users/jrestall/Dev/underkill-stack/node_modules/.pnpm/[email protected][email protected][email protected]_bnl2udvc6mytkjbq3r7oxas3x4/node_modules/lib/router/utils.ts:454:12)
    at Array.map (<anonymous>)
    at convertRoutesToDataRoutes (file:///Users/jrestall/Dev/underkill-stack/node_modules/.pnpm/[email protected][email protected][email protected]_bnl2udvc6mytkjbq3r7oxas3x4/node_modules/lib/router/utils.ts:430:17)
    at createStaticRouter (file:///Users/jrestall/Dev/underkill-stack/node_modules/.pnpm/[email protected][email protected][email protected]_bnl2udvc6mytkjbq3r7oxas3x4/node_modules/lib/dom/server.tsx:281:20)
    at ServerRouter (file:///Users/jrestall/Dev/underkill-stack/node_modules/.pnpm/[email protected][email protected][email protected]_bnl2udvc6mytkjbq3r7oxas3x4/node_modules/lib/dom/ssr/server.tsx:68:16)
    at renderWithHooks (/Users/jrestall/Dev/underkill-stack/node_modules/.pnpm/[email protected][email protected]/node_modules/react-dom/cjs/react-dom-server.node.production.js:3769:18)
    at renderElement (/Users/jrestall/Dev/underkill-stack/node_modules/.pnpm/[email protected][email protected]/node_modules/react-dom/cjs/react-dom-server.node.production.js:3907:14)
@jrestall jrestall added the bug label Oct 6, 2024
@jacobparis
Copy link

I think I agree that react-router should match react's behaviour in cases where NODE_ENV is not set

though in a vacuum I do think loading the production build would have been a better default for both

@brophdawg11
Copy link
Contributor

What is the ask here? To add that to the template? Or to Jacob's underkill stack?

If the former, would you care to submit a PR?

@jacobparis
Copy link

I think the ask is for react-router-serve to not set NODE_ENV to production, because react router cannot tell react to run the production build and therefore will be out of sync with it

@jrestall
Copy link
Contributor Author

What is the ask here? To add that to the template? Or to Jacob's underkill stack?

If the former, would you care to submit a PR?

Neither, I'd prefer devs don't need to even think about it and we handle setting node_env automatically in packages/react-router-serve/bin.js to ensure it is consistent and set before any react module is loaded by the react-router-serve cli code.

Here's my proposed PR fix for the bin.js change #12193.

I also found this error is only reproducible with React v19 but even with React v18 the dev react and prod react-dom modules are loaded, it just doesn't hard error. So I think still a good idea to fix.

@brophdawg11
Copy link
Contributor

I'd prefer aligning our default with Reacts - but will run this by the team. The side-effect nature of the linked PR could make it very tricky to diagnose why React was loading the production build if you were never setting NODE_ENV in your environment.

@brophdawg11
Copy link
Contributor

This should be fixed by #12578 and available in the next release

@brophdawg11 brophdawg11 added the awaiting release This issue have been fixed and will be released soon label Dec 18, 2024
@brophdawg11 brophdawg11 linked a pull request Dec 18, 2024 that will close this issue
Copy link
Contributor

🤖 Hello there,

We just published version 7.1.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

fumi321 pushed a commit to fumi321/tailwind-color-contrast-grid that referenced this issue Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting release This issue have been fixed and will be released soon bug
Projects
None yet
3 participants