fix: opt out of direct import in SSR when the file is not JS #8454
Conversation
| @@ -291,6 +294,10 @@ async function nodeImport( | |||
| ? { ...resolveOptions, tryEsmOnly: true } | |||
| : resolveOptions | |||
| ) | |||
| // These cannot be imported directly | |||
| if (url.match(/\.(css|less|sass|scss|styl|stylus|pcss|postcss|json)$/)) { | |||
There was a problem hiding this comment.
duplicating regex like these over the codebase can lead to errors down the road where it is changed in one place but forgotten in others. Is there a way to put it into a common place like with known js extensions?
vite/packages/vite/src/node/utils.ts
Line 228 in 164f528
There was a problem hiding this comment.
I agree but I don't think marko, svelte, or astro files can be directly imported by Node, can they? If we use isJSRequest nodeImport will try a direct import. It would also miss wasm files (I don't know whether it's desirable or not). I couldn't find a good regex for this anywhere in the code.
How about actually trying to import directly and catching ERR_UNKNOWN_FILE_EXTENSION to try again with Vite's loaders? This way we could support custom Node loaders.
cyco130
force-pushed
the
fix-ssr-css-import
branch
from
5893a4d to
b6cbf82
Compare
✅ Deploy Preview for vite-docs-main canceled.
|
| @@ -690,7 +690,7 @@ export function tryNodeResolve( | |||
| // otherwise we may introduce duplicated modules for externalized files | |||
| // from pre-bundled deps. | |||
| if (!isBuild) { | |||
| const versionHash = depsOptimizer.metadata({ ssr }).browserHash | |||
| const versionHash = depsOptimizer.metadata({ ssr })?.browserHash | |||
There was a problem hiding this comment.
This is always defined, why was this change needed?
There was a problem hiding this comment.
It isn't defined when the module is CSS (tried with sanitize.css) maybe because it can't be optimized? This is new by the way, the previous version didn't need this.
There was a problem hiding this comment.
I tried this repro https://github.com/cyco130/vite-ssr-react/tree/ssr-css-bug, against beta.5 and it is working 🤔
Could you create a failing repro so I can check what is going on here?
There was a problem hiding this comment.
I found why you need this, I'll send a PR tomorrow to fix it. Still a repro of the CSS issue against latest would be good
There was a problem hiding this comment.
Could you create a failing repro so I can check what is going on here?
I updated the repro now and it is failing for me. Maybe try rm -rf node_modules/.vite?
cyco130
force-pushed
the
fix-ssr-css-import
branch
from
b6cbf82 to
fba2ba8
Compare
|
I tested the repro with Vite 2.9.13 and plugin-react 1.3.2, and it seems to work fine. Stackblitz. So this might be a regression in Vite 3. |
Description
Currently importing external CSS files in SSR throws
ERR_UNKNOWN_FILE_EXTENSIONunless those modules are marked as external (reproduction, StackBlitz). This PR solves it by opting out of direct Node import if the resolved file has a non-JS extension.Additional context
The regex testing if a file is a non-JS file (which I copied from the optimizer code) may require some more thinking.
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123).