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

Remove ssr.noExternal from SPA template #8952

Merged
merged 1 commit into from Mar 1, 2024

Conversation

markdalgleish
Copy link
Member

@markdalgleish markdalgleish commented Mar 1, 2024

The SPA template is currently broken due to this setting. In dev mode, I get the following error:

[vite] Internal server error: module is not defined
      at eval (/path/to/project/node_modules/react/jsx-dev-runtime.js:8:3)
      at instantiateModule (file:///path/to/project/node_modules/vite/dist/node/chunks/dep-jDlpJiMN.js:54725:15)

During the build I also see this:

x Build failed in 124ms
Error [RollupError]: [commonjs--resolver] Cannot bundle Node.js built-in "stream" imported from "node_modules/react-dom/cjs/react-dom-server-legacy.node.production.min.js". Consider disabling ssr.noExternal or remove the built-in dependency.
file: /path/to/project/node_modules/react-dom/server.node.js

Rather than simply removing ssr.noExternal, I tried to fix it by also setting ssr.external to avoid bundling troublesome dependencies. I was able to fix it with the following config:

ssr: {
  noExternal: true,
  external: [
    "react",
    "react-dom",
    "@remix-run/server-runtime",
  ],
}

I thought this might be good enough, but I decided to try introducing react-bootstrap since this was package that originally triggered this change due to Rollup build errors. I then received the following error:

[vite] Internal server error: window is not defined
      at eval (/path/to/project/node_modules/classnames/index.js:77:3)
      at eval (/path/to/project/node_modules/classnames/index.js:79:2)
      at instantiateModule (file:///path/to/project/node_modules/vite/dist/node/chunks/dep-jDlpJiMN.js:54725:15)

I can fix this in dev by adding react-bootstrap to the ssr.external array, but then we're back where we started with a broken build. Instead, I had to add the lower-level offending packages to get both dev and build to work:

ssr: {
  noExternal: true,
  external: [
    "react",
    "react-dom",
    "@remix-run/server-runtime",
    "classnames",
    "@restart/ui",
  ],
}

So taking a step back, this felt like we had replaced one problem with a worse one, since the original issue with react-bootstrap can be fixed more succinctly and directly like this:

ssr: {
  noExternal: ["react-bootstrap"],
}

So I think ultimately it's a safer bet for us to leave this area unconfigured for now unless we can find a better way to avoid these sorts of issues.

Either way, this PR can be thought of as a hotfix to get the template back to a working state.

Copy link

changeset-bot bot commented Mar 1, 2024

⚠️ No Changeset found

Latest commit: a0d3a7e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@markdalgleish markdalgleish merged commit 2228ec3 into main Mar 1, 2024
5 checks passed
@markdalgleish markdalgleish deleted the markdalgleish/remove-ssr-noexternal branch March 1, 2024 04:16
IgnusG pushed a commit to IgnusG/remix that referenced this pull request Mar 4, 2024
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.

None yet

1 participant