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

ssrLoadModule fails to respect preact/compat aliases, while the SSR bundle does respect preact/compat aliases #15503

Closed
jsamilow opened this issue Jan 3, 2024 · 11 comments · Fixed by #15602

Comments

@jsamilow
Copy link

jsamilow commented Jan 3, 2024

Describe the bug

I'm migrating an existing repo from Webpack to Vite.

We use preact, and rely on aliasing react and react-dom to preact/compat in the SSR bundle. Vite handles the aliases as expected when it bundles the SSR code for production. But when loading SSR for development using vite.ssrLoadModule, Vite fails to respect the react->preact/compat alias, even though it respects other aliases in vite.config.js. I have provided a minimalist reproduction of the issue below.

If this is not a bug and is in fact the expected functionality, the discrepancy between prod and dev is unexpected to say the least. I was debating whether this is a Vite or Preact issue, but the fact that the behavior differs within Vite leads me to think it is a Vite issue.

For some background information, which is not necessary to understand the problem, I found that SSR worked when I loaded it from a bundle:

const ssrModule = await import('./web_ssr/ssr.js);

but failed when I loaded it using ssrLoadModule('/frontend/ssr.js') with

 [Error: [object Object] is not a valid HTML tag name in <[object Object] value="[object Object]">] [object Object] is not a valid HTML tag name in <[object Object] value="[object Object]"> Error: [object Object] is not a valid HTML tag name in <[object Object] value="[object Object]">
    _renderToString (node_modules/preact-render-to-string/src/index.js:316:3)
    Proxy.m (node_modules/preact-render-to-string/src/index.js:59:14)

The issue is that React's renderer is incompatible with preact-render-to-string. In the bundle case, Preact's renderer is used as desired, but in the ssrLoadModule case, React's renderer is used.

Reproduction

https://stackblitz.com/edit/vitejs-vite-nxei8z

Steps to reproduce

npm install --legacy-peer-deps

To run it in production mode, first do npm run build:vite-ssr to create the SSR bundle. Then NODE_ENV=prod node -r esbuild-runner/register launch.js. You should see it print react version 17.0.2. This is what's expected, as preact/compat provides a hardcoded version of 17.0.2.

To run it in development mode, do NODE_ENV=development node -r esbuild-runner/register launch.js. You should see it print react version 16.14.0. This is not expected, and indicates that Vite failed to map react to preact/compat.

The discrepancy in versions between the bundle and ssrLoadModule indicates that different libraries are being loaded.

But strangely, ssrLoadModule does respect the testAlias that I added in vite.config.js. If you comment out that alias, everything breaks.

I'm skeptical this can be a bug given how dead simple it is to reproduce, but I cannot find anything in the documentation or Preact SSR examples that predicts this discrepancy between bundle and development.

System Info

px envinfo --system --npmPackages '{vite,@vitejs/*,rollup}' --binaries --browsers
Need to install the following packages:
  envinfo@7.11.0
Ok to proceed? (y) y

  System:
    OS: macOS 13.6
    CPU: (10) arm64 Apple M1 Pro
    Memory: 111.80 MB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 16.18.1 - ~/.nvm/versions/node/v16.18.1/bin/node
    npm: 8.19.2 - ~/.nvm/versions/node/v16.18.1/bin/npm
  Browsers:
    Chrome: 120.0.6099.129
    Safari: 16.6
  npmPackages:
    @vitejs/plugin-react: ^4.2.0 => 4.2.1
    vite: ^5.0.10 => 5.0.10


### Used Package Manager

npm

### Logs

_No response_

### Validations

- [X] Follow our [Code of Conduct](https://github.com/vitejs/vite/blob/main/CODE_OF_CONDUCT.md)
- [X] Read the [Contributing Guidelines](https://github.com/vitejs/vite/blob/main/CONTRIBUTING.md).
- [X] Read the [docs](https://vitejs.dev/guide).
- [X] Check that there isn't [already an issue](https://github.com/vitejs/vite/issues) that reports the same bug to avoid creating a duplicate.
- [X] Make sure this is a Vite issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to [vuejs/core](https://github.com/vuejs/core) instead.
- [X] Check that this is a concrete bug. For Q&A open a [GitHub Discussion](https://github.com/vitejs/vite/discussions) or join our [Discord Chat Server](https://chat.vitejs.dev/).
- [X] The provided reproduction is a [minimal reproducible example](https://stackoverflow.com/help/minimal-reproducible-example) of the bug.
Copy link

stackblitz bot commented Jan 3, 2024

Fix this issue in StackBlitz Codeflow Start a new pull request in StackBlitz Codeflow.

@hi-ogawa
Copy link
Collaborator

hi-ogawa commented Jan 4, 2024

It looks like currently Vite tries to externalize react during dev since it's accessible in node_modules. Assuming you still need react to be available as a project's dependency outside of Vite, one quick workaround would be to add these aliases to ssr.noExternal: ["react"]. I updated your repro to illustrate the usage:

https://stackblitz.com/edit/vitejs-vite-mudrh8?file=repro.mjs

I'm not sure but maybe Vite can safely do this automatically. For example, shouldExternalizeForSSR check during import analysis could be skipped if specifier matches resolve.alias pattern?

// skip ssr external
if (ssr) {
if (shouldExternalizeForSSR(specifier, importer, config)) {

@jsamilow
Copy link
Author

jsamilow commented Jan 4, 2024

I don't have a good sense of whether it's safe or even desirable to automatically skip externalizing packages that match an alias. Maybe it would just be better if the documentation were more robust around this point, and also mentioned common differences between building SSR for prod and ssrLoadModule in dev.

Screenshot 2024-01-04 at 12 54 13 PM

I saw this in the documentation, but didn't grasp that this warning only applies to dev, not prod bundling. I also didn't realize that this is implying one should use an alias system independent of Vite because Vite won't respect the alias.

After adding react and react-dom to ssr.noExternal, I can see that import react in frontend/ssr.js now resolves to preact/compat instead of react. Strangely enough, the exact same {"error":"[object Object] is not a valid HTML tag name in <[object Object] value=\"[object Object]\">"} error still happens though.

Is it possible that noExternal: [react, react-dom] fails to intercept all react imports within node_modules? As in, could there be subdependencies that are still referencing react instead of preact/compat? Must all dependencies that reference react also be non-externalized in order to have import react resolve to preact/compat? For example, if I log React.version in the react-select library, it gives React select 16.14.0, showing that react and not preact/compat is what was loaded.

@hi-ogawa
Copy link
Collaborator

hi-ogawa commented Jan 4, 2024

Is it possible that noExternal: [react, react-dom] fails to intercept all react imports within node_modules? As in, could there be subdependencies that are still referencing react instead of preact/compat?

Good point. I haven't thought about that, but I think that's what's explained by that warning https://vitejs.dev/guide/ssr.html#ssr-externals, meaning that for example react-select is not processed by Vite as its "SSR externalized dependency" so import "react" inside react-select would be left as is.

Speaking of dev/prod difference, I think your output js file ./web_ssr/ssr.js still keeps imports such as import "react-select" without bundled. The reason why prod version works might be because there is other mechanism (I suppose moduleAliases?) to alias react -> preact inside react-select during runtime.

Further trick to mitigate this would be to put such sub dependencies into noExternal as well (or put everything as noExternal: true?). But as the warning message says, it might be simpler if you can use package-manager based package alias.

@jsamilow
Copy link
Author

jsamilow commented Jan 9, 2024

I got past the error by setting:

    "react": "npm:@preact/compat",
    "react-dom": "npm:@preact/compat",

in package.json. I'm now certain that all node_modules are referencing preact instead of react. In fact, if I delete the aliases in vite.config.js the bundle still works, which is exactly what you'd expect.

However, the ssrLoadModule path is broken with:

[app][WARN] [TypeError: Cannot read properties of null (reading 'context')] Cannot read properties of null (reading 'context') Object.useContext (node_modules/preact/hooks/src/index.js:343:36)
useDevice (node_modules/react-responsive/dist/webpack:/dist/react-responsive.js:132:73)
Proxy.useMediaQuery (node_modules/react-responsive/dist/webpack:/dist/react-responsive.js:218:24)
Module.useMobile (frontend/main/useResponsive.ts:10:32)
Object.call (frontend/components/nav.jsx:128:43)
renderFunctionComponent (file://node_modules/preact-render-to-string/src/index.js:119:25)
_renderToString (file://node_modules/preact-render-to-string/src/index.js:282:16)
_renderToString (file://node_modules/preact-render-to-string/src/index.js:399:15)
_renderToString (file://node_modules/preact-render-to-string/src/index.js:298:15)
_renderToString (file://node_modules/preact-render-to-string/src/index.js:298:15)
_renderToString (file://node_modules/preact-render-to-string/src/index.js:256:5)
_renderToString (file://node_modules/preact-render-to-string/src/index.js:298:15)
_renderToString (file://node_modules/preact-render-to-string/src/index.js:298:15)
_renderToString (file://node_modules/preact-render-to-string/src/index.js:298:15)
_renderToString (file://node_modules/preact-render-to-string/src/index.js:256:5)
_renderToString (file://node_modules/preact-render-to-string/src/index.js:298:15)
_renderToString (file://node_modules/preact-render-to-string/src/index.js:298:15)
_renderToString (file://node_modules/preact-render-to-string/src/index.js:298:15)
_renderToString (file://node_modules/preact-render-to-string/src/index.js:298:15)
_renderToString (file://node_modules/preact-render-to-string/src/index.js:298:15)
_renderToString (file://node_modules/preact-render-to-string/src/index.js:298:15)
F (file://node_modules/preact-render-to-string/src/index.js:256:5)

which means that something is fundamentally broken in preact, i.e. that currentComponent is not set in preact/hooks. The vite build --ssr path still works.

I posted on the preact boards, and someone on Preact's core team wrote:

I’m pretty sure that this is is caused by two instances of Preact being active. Could be that vite is bundling one copy somewhere but doesn’t at another part."

It's surprising that the bundle path and development path can differ so dramatically.

I'll note that preact is imported by my application prior to ssrLoadModule being called. Maybe the preact instance used prior to ssrLoadModule is different from the preact instance used by ssrLoadModule?

@hi-ogawa
Copy link
Collaborator

hi-ogawa commented Jan 10, 2024

It's a little difficult to diagnose without concrete reproduction, so I would appreciate if you can provide one.

On a related note, one thing come to my mind is that, having "dependencies": { "react": "npm:@preact/compat" } might only apply to your own package as an alias, but not for for sub dependencies.

For example, pnpm has overrides option to swap package for entire dependencies https://pnpm.io/package_json#pnpmoverrides, so using this option might help if you haven't already.

@jsamilow
Copy link
Author

It seems like npm also supports overrides:

  "overrides": {
    "react": "$react",
    "react-dom": "$react-dom"
  },

This doesn't help though. I'll work on a reproduction.

@jsamilow
Copy link
Author

jsamilow commented Jan 17, 2024

https://github.com/jsamilow/preact-vite-error-repro
https://probable-system-57xrxp744r4h7pv.github.dev/

Here's my effort at a minimalistic reproduction. Unfortunately, this repro breaks for both dev and prod, whereas in the real code it only breaks in dev. Nevertheless, the issue is the same—currentComponent is null inside preact/hooks—so I'm hoping that understanding this will still be useful.

Steps:

npm i
NODE_ENV=development node -r esbuild-runner/register  launch.js

You can do

npx vite build --ssr ssr.js --outDir web_ssr
NODE_ENV=production node -r esbuild-runner/register  launch.js

for the production path. You should see:

An error occurred: TypeError: Cannot read properties of undefined (reading 'context')
    at exports.useContext (preact-vite-error-repro/node_modules/preact/hooks/src/index.js:344:36)
    at useDevice (preact-vite-error-repro/node_modules/react-responsive/dist/webpack:/MediaQuery/src/useMediaQuery.ts:37:39)
    at Module.useMediaQuery (preact-vite-error-repro/node_modules/react-responsive/dist/webpack:/MediaQuery/src/useMediaQuery.ts:107:26)

@hi-ogawa
Copy link
Collaborator

hi-ogawa commented Jan 18, 2024

Hey, thanks for getting back with the reproduction. However, your repro still looked complicated, so I stripped out quite a lot and ended up with this https://github.com/hi-ogawa/repro-vite-preact-15503, which hopefully shows the same error you're experiencing.

My observation so far is that it's probably esm/cjs dual package issue of preact https://nodejs.org/api/packages.html#dual-commonjses-module-packages where your own code is using esm-version of preact but react-responsive is using cjs-version of preact, thus leading to two instances of preact.

To make this work on dev, I think you would need to manage ssr.optimizeDeps to pre-bundle cjs dependencies into esm. Probably this is very tricky to get it right, but it looks like this hi-ogawa/repro-vite-preact-15503#1

To make this work on build, I think it's easier because you have more control on build output (compared to dev where vite forces user code to use import). As shown in https://github.com/hi-ogawa/repro-vite-preact-15503, one way to do this would be to "re-transpile" Vite's ESM output into CJS e.g. by esbuid --format=cjs. Another approach would be that you can bundle entire thing and still keep esm by Vite via ssr.noExternal: true or again by esbuild cli esbuild --bundle.

And finally it's probably a little crazy idea. I was wondering why react doesn't experience this kind of issue on Vite SSR, then I realized react is cjs only package. In hi-ogawa/repro-vite-preact-15503#2, I explore this idea and patched preact packages to remove esm exports from their package.json and it actually seems to work...


I just tried to give as much information as I can, but employing any of these tricks in the actual project might be difficult. Overall, I feel this is preact ecosystem issue/limitation in terms of react compatibility (while Vite SSR still provides some sort of workaround), so I might not be able to help this case further. But I hope you got some idea about the underlying issue.

@jsamilow
Copy link
Author

jsamilow commented Jan 22, 2024

Thank you for your reply and persistence in investigating this!

You are exactly right about the problem, and patching node_modules/preact/package.json to remove the ESM imports fixes the error.

I believe there are interesting subtleties here, and I hope elaborating on them will provide future help to anyone dealing with an inscrutable dual package error.

While react-responsive@9.0.2 only provides CJS imports, I noticed that there is a beta version that provides ESM imports as well. I expected that if I switched to this beta version, the error would go away without patching preact, because Vite would load the ESM version of react-responsive as long as it existed.

But I was wrong. Vite chose to load the CJS version of react-responsive instead of the ESM version declared in package.json. I downloaded the Vite source code and added logging lines here prove that this is what was happening.

dynamic import url and id {
  id: 'preact',
  url: 'file:///Users/jared/apps/project/node_modules/preact/dist/preact.mjs'
}
...
...
dynamic import url and id {
  id: 'react-responsive',
  url: 'file:///Users/jared/apps/project/node_modules/react-responsive/dist/react-responsive.js'
}

You can see that the ESM version of preact is imported, while the CJS version of react-responsive is imported.

It looks looks like vite ignores package.json's main.module field and only cares about exports.
To demonstrate this using your repro, install npm i react-responsive@10.0.0-beta.1 and apply the following patch to react-responsive/package.json:

index 824d442..ccf042a 100644
--- a/node_modules/react-responsive/package.json
+++ b/node_modules/react-responsive/package.json
@@ -11,6 +11,13 @@
   "license": "MIT",
   "main": "./dist/cjs/index.js",
   "module": "./dist/esm/index.js",
+  "type": "module",
+  "exports": {
+    ".": {
+      "import": "./dist/esm/index.js",
+      "require": "./dist/cjs/index.js"
+    }
+  },
   "types": "./dist/types/index.d.ts",
   "sideEffects": false,
   "files": [

(I can push a branch to your repro if you grant me permission but this modification is so small I don't think I need to push it.)

You will find that the code is no longer broken.

It's surprising to me that Vite's resolution in resolveExportsOrImports logic will use exports.import if it is available even with type: commonjs, but ignores main.module in favor of the main import unless type: module is present. So in this case, Vite loads ESM preact but CJS react-responsive, even though both packages are type: commonjs.

I would have expected Vite to always prefer a package's ESM version as long as it is available. react-laag was another library where I saw this issue, and sure enough, it provides both imports yet Vite was importing the CJS one anyway.

I am not sure this can be described as a bug, because there might be a good reason for Vite to resolve modules using this logic. But the behavior is surprising at very least.

Although Vite removed the vite.config option for ssr.format = cjs in Vite 5, am I correct that this option is ignored when resolving modules for SSR in dev? Seems to me like it has no effect on Vite's decision to import ESM/CJS for preact and react-responsive

Thanks once again for looking deeply at this.

@rschristian
Copy link
Contributor

It's surprising to me that Vite's resolution in resolveExportsOrImports logic will use exports.import if it is available even with type: commonjs, but ignores main.module in favor of the main import unless type: module is present. So in this case, Vite loads ESM preact but CJS react-responsive, even though both packages are type: commonjs.

Keep in mind that, since package.json#module is ignored by Node, it essentially became a bundler-only ESM entry, meant entirely for browser use. It does not (and does not need to) adhere to Node's ESM semantics.

Even if you fixed that by preprocessing these modules, many do access browser globals in these entry points. Trying to consume this entry might be more headaches than it's worth. That library should be using package.json#exports

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants