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

Decouple CLI arg parsing/execution for NODE_ENV initialization #12505

Closed
wants to merge 11 commits into from

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Dec 9, 2024

It seems React 19 is a bit more strict with NODE_ENV and when it's not set by the user there is currently a mismatch between our code and React's code (similar to the issue in #12078)

  • We default to production when we call vite.resolveConfig in in plugin.ts
  • But React defaults to development

This mismatch causes issues because we end up with mismatched versions of React in the bundle versus the react-router build runtime.

The import chain to get to react is run.ts -> commands.ts -> plugin.ts -> react-router -> react, and we probably want our default value to be conditional based on the command (similar to what vite does). This PR does 2 things:

  • Extracts the loadPluginContext out to it's own plugin-context.ts file so that it can be loaded from commands.ts without also loading react-router which loads react and evaluates the value of NODE_ENV
  • Extracts the argument parsing out of run.ts into a new parse.ts so we can run that up front in isolation, then set the proper NODE_ENV based on the command

This way, running the CLI will not load react-router/react until after we've properly initialized NODE_ENV

Closes #12138, #12078

Copy link

changeset-bot bot commented Dec 9, 2024

🦋 Changeset detected

Latest commit: 2fe399b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 11 packages
Name Type
@react-router/serve Patch
@react-router/dev Patch
@react-router/fs-routes Patch
@react-router/remix-routes-option-adapter Patch
create-react-router Patch
react-router Patch
react-router-dom Patch
@react-router/architect Patch
@react-router/cloudflare Patch
@react-router/express Patch
@react-router/node Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment on lines +4 to +13
let { input, flags, command } = parseArgs();

// If not already set, default `NODE_ENV` so React loads the proper
// version in it's CJS entry script. We have to do this before importing `run.ts`
// since that is what imports `react` (indirectly via `react-router`)
if (command === "dev") {
process.env.NODE_ENV = process.env.NODE_ENV ?? "development";
} else {
process.env.NODE_ENV = process.env.NODE_ENV ?? "production";
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parse args and set NODE_ENV first

Comment on lines 15 to 25
import("./run").then(({ run }) => {
run(input, flags, command).then(
() => {
process.exit(0);
},
(error: unknown) => {
if (error) console.error(error);
process.exit(1);
}
);
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then run the CLI

* Parse command line arguments for `react-router` dev CLI
*/
export function parseArgs(argv: string[] = process.argv.slice(2)) {
// Check the node version
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No changes to this logic, just extracted out from run() below


// If not already set, default `NODE_ENV=production` so React loads the proper
// version in it's CJS entry script
process.env.NODE_ENV = process.env.NODE_ENV ?? "production";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set this correctly before calling the CLI so the proper version of React gets loaded (#12078)

@@ -9,6 +9,7 @@ import pkg from "./package.json";

const entry = [
"cli/index.ts",
"cli/run.ts",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed to make run it's own entry point so it can be lazily imported as runtime

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm, well that didn't fix CI so may need to dig into this part a bit deeper

@brophdawg11
Copy link
Contributor Author

Closing this because this reorg causes a node 22.12 cyclical dependency issue in vite dev (not present in node 22.10). Going to go with a more streamlined approach in a new PR

@brophdawg11 brophdawg11 deleted the brophdawg11/node-env branch December 18, 2024 15:03
@brophdawg11
Copy link
Contributor Author

New PR: #12578

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants