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.

Sign up for GitHub

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

fix: don't store temporary vite config file in node_modules if deno #18823

Merged
merged 3 commits into from
Nov 29, 2024
Merged

fix: don't store temporary vite config file in node_modules if deno #18823

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f84777f
fix: dont store in node_modules if deno
kazushisan Nov 28, 2024
91febc0
refactor: simplify condition for deno
kazushisan Nov 29, 2024
d6ae93e
chore: add comment for deno condition
kazushisan Nov 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion packages/vite/src/node/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1858,7 +1858,13 @@ async function loadConfigFromBundledFile(
// with --experimental-loader themselves, we have to do a hack here:
// write it to disk, load it with native Node ESM, then delete the file.
if (isESM) {
Copy link
Member

Choose a reason for hiding this comment

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

const hasNpmPrefixExternalImport = Object.values(result.metafile.outputs)
    .flatMap((output) => output.imports)
    .filter((imp) => imp.external && imp.path.startsWith('npm:'))

Does passing this value from bundleConfigFile and only avoid writing to node_modules in that case work? If I understand correctly, the problem only happens if the config file has an import with npm: prefix.

If you have a code like below, it won't work. But this is already not supported, so it should be fine.

const dynImport = (str: string) => import(str)

dynImport('npm:foo')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand correctly, the problem only happens if the config file has an import with npm: prefix.

Yes, you're right! Here's a break down of what's going to happen for clarity.

  1. Vite removes the npm: prefix from import statements
  2. bundled file is executed with Deno
  3. Deno's module resolution expects an npm: prefix when we want to do something like import { defineConfig } from 'vite';
  4. However since this bundled file is placed in node_modules/, Deno will consider the file as an Node.js compatible module and resolve import statements accordingly
  5. import works!

So your solution should work, but I think it make thing too unpredictable and fragile because it's like trying to implement a transformation layer that voids Deno's behavior difference for modules inside/outside of node_modules/

Since the bundled file is going to be executed by Deno not Node, I'd say that keeping the import statements as-is is a better solution.

const nodeModulesDir = findNearestNodeModules(path.dirname(fileName))
// Storing the bundled file in node_modules/ is avoided for Deno
// because Deno only supports Node.js style modules under node_modules/
// and configs with `npm:` import statements will fail when executed.
const nodeModulesDir =
typeof process.versions.deno === 'string'
? undefined
: findNearestNodeModules(path.dirname(fileName))
if (nodeModulesDir) {
await fsp.mkdir(path.resolve(nodeModulesDir, '.vite-temp/'), {
recursive: true,
Expand Down