Fail on cjs files in a node_modules folder somewhere out of the project

[Jinjiang]

Hi, first of all, a really nice job!

I just came across an issue when I have to import a cjs file in another node_modules folder out of the target project. The error message is like:

caught SyntaxError: The requested module '/@fs/Users/jinjiang/xxx/packages/bar/node_modules/foo/index.js?v=f73c5dad' does not provide an export named 'foo' (at main.js?t=1682654369388:8:10)

The minimized reproduction is here to refer to:
https://github.com/Jinjiang/reproductions/tree/vite-plugin-commonjs-20230428

As it mentions in the main.js:

// // this would work
// import { foo } from './foo'

// // this would work
// import { foo } from '../foo'

// this would fail
import { foo } from '../bar/node_modules/foo'

Thanks.

[caoxiemeihao]

Maybe this way can solution the problem for you 🤔

https://github.com/vite-plugin/vite-plugin-commonjs/pull/20/files#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80R63

commonjs({
  filter(id) {
    if (id.includes('node_modules/foo')) {
      return true
    }
  },
})

---

If this doesn't work, I'll iterate on the next version.

[Jinjiang]

@caoxiemeihao I tried again. Unfortunately, it doesn't work. Not sure where the broken point of the logic is.

[caoxiemeihao]

@Jinjiang Okay! I'll try to Debug it.

[Jinjiang]

Update: I debugged a little bit and found this case hit 2 walls at the same time:

1. /node_modules/ which might be skipped by filter
2. extensions (the native path.extname is only for file systems which can't handle the query string like xxx.js?v=f73c5dad)

However, I don't have a big picture of your tech design yet. Not sure which way to solve this is the best and won't involve other issues. So I'd like to leave the eventual bugfix to you.

Thanks.

[caoxiemeihao]

It works to me @ v0.7.0 - coming soon

大佬看看代码不(帮忙 review 下呢 🥺)，放假回家了先，明天下午 or 晚上有时间我发个版本

[Jinjiang]

Would you mind checking one more case that I found? It's about package regexpu-core. If we have import rewritePattern from '../other-project/node_modules/regexpu-core', it will get this error:

[vite] Internal server error: invalid import "require(`regenerate-unicode-properties/${ path }.js`)". Variable bare imports are not supported, imports must start with ./ in the static part of the import. For example: import(`./foo/${bar}.js`).

However, import rewritePattern from 'regexpu-core' has no such a problem.

This is also about requests to other node_modules. So I just put it in this same GitHub issue.

Thanks.

[caoxiemeihao]

v0.7.0 is out, can you re-provide a minimal repo and based on v0.7.0 😂

[Jinjiang]

I'm afraid the error is still there on v0.7.0 as:

Uncaught SyntaxError: The requested module '/@fs/Users/xxxx/reproductions/packages/bar/node_modules/foo/index.js?v=d571362a' does not provide an export named 'foo' (at main.js?t=1682991288619:8:10)

Here is the reproduction
https://github.com/Jinjiang/reproductions/tree/vite-plugin-commonjs-20230502

I added the case about regexpu-core there too.

Thanks.

[caoxiemeihao]

1. This error comes from the cache.

Uncaught SyntaxError: The requested module '/@fs/Users/xxxx/reproductions/packages/bar/node_modules/foo/index.js?v=d571362a' does not provide an export named 'foo' (at main.js?t=1682991288619:8:10)

2. regexpu-core requires support mono-repo(pnpm) - TODO 🔨

[Jinjiang]

@caoxiemeihao I tried it from a brand-new workspace, including re-generated lock-file or not. The first error was still there. Would you mind double-checking?

[caoxiemeihao]

Maybe you forgot to explicitly include node_modules
https://github.com/vite-plugin/vite-plugin-commonjs/tree/v0.7.0#node_modules

commonjs({
+ filter(id) {
+   if (id.includes('node_modules/foo')) {
+     return true
+   }
+ },
})

[Jinjiang]

Understood. It worked after I added the config. Thanks.

[caoxiemeihao]

@Jinjiang 👋
Everything is right, except that our small range of condition causes problems :(

// It works on my local machine 😆
commonjs({
  filter(id) {
    if (id.includes('node_modules/foo')) {
      return true
    }
    if (id.includes('node_modules/regexpu-core')) {
      return true
    }
    if (id.includes('node_modules/regenerate-unicode-properties')) {
      return true
    }
  },
}),

So perhaps we can be lazy

commonjs({
  filter(id) {
    if (id.includes('node_modules')) {
      return true
    }
  },
}),

---

regexpu-core internally imported regenerate-unicode-properties

[Jinjiang]

@caoxiemeihao I added the filter config and updated the reproduction repo. The error was still there. And this time the terminal also had errors:

[plugin:vite-plugin-commonjs] invalid import "require(`regenerate-unicode-properties/${ path }.js`)". Variable bare imports are not supported, imports must start with ./ in the static part of the import. For example: import(`./foo/${bar}.js`).
/xxx/reproductions/node_modules/.pnpm/regexpu-core@5.3.2/node_modules/regexpu-core/rewrite-pattern.js

$ pnpm dev

> reproduce@ dev /xxx/reproductions
> pnpm --filter vite-project dev


> vite-project@0.0.0 dev /xxx/reproductions/packages/vite-project
> vite


  VITE v4.3.2  ready in 201 ms

  ➜  Local:   http://localhost:5173/
  ➜  Network: use --host to expose
  ➜  press h to show help
Error:   Failed to scan for dependencies from entries:
  /xxx/reproductions/packages/vite-project/index.html

  ✘ [ERROR] invalid import "require(`regenerate-unicode-properties/${ path }.js`)". Variable bare imports are not supported, imports must start with ./ in the static part of the import. For example: import(`./foo/${bar}.js`). [plugin vite-plugin-dynamic-import:pre-bundle]

    ../../node_modules/.pnpm/esbuild@0.17.18/node_modules/esbuild/lib/main.js:1421:21:
      1421 │         let result = await callback({
           ╵                      ^

    at dynamicImportToGlob (file:///xxx/reproductions/node_modules/.pnpm/vite-plugin-dynamic-import@1.3.2/node_modules/vite-plugin-dynamic-import/dist/index.mjs:321:11)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async globFiles (file:///xxx/reproductions/node_modules/.pnpm/vite-plugin-dynamic-import@1.3.2/node_modules/vite-plugin-dynamic-import/dist/index.mjs:508:10)
    at async DynaimcRequire.generateRuntime (file:///xxx/reproductions/node_modules/.pnpm/vite-plugin-commonjs@0.7.0/node_modules/vite-plugin-commonjs/dist/index.mjs:284:26)
    at async transformCommonjs (file:///xxx/reproductions/node_modules/.pnpm/vite-plugin-commonjs@0.7.0/node_modules/vite-plugin-commonjs/dist/index.mjs:430:20)
    at async file:///xxx/reproductions/node_modules/.pnpm/vite-plugin-commonjs@0.7.0/node_modules/vite-plugin-commonjs/dist/index.mjs:379:30
    at async requestCallbacks.on-load (/xxx/reproductions/node_modules/.pnpm/esbuild@0.17.18/node_modules/esbuild/lib/main.js:1421:22)
    at async handleRequest (/xxx/reproductions/node_modules/.pnpm/esbuild@0.17.18/node_modules/esbuild/lib/main.js:723:13)

  This error came from the "onLoad" callback registered here:

    ../../node_modules/.pnpm/esbuild@0.17.18/node_modules/esbuild/lib/main.js:1279:20:
      1279 │       let promise = setup({
           ╵                     ^

    at setup (file:///xxx/reproductions/node_modules/.pnpm/vite-plugin-commonjs@0.7.0/node_modules/vite-plugin-commonjs/dist/index.mjs:372:17)
    at handlePlugins (/xxx/reproductions/node_modules/.pnpm/esbuild@0.17.18/node_modules/esbuild/lib/main.js:1279:21)
    at buildOrContextImpl (/xxx/reproductions/node_modules/.pnpm/esbuild@0.17.18/node_modules/esbuild/lib/main.js:968:5)
    at Object.buildOrContext (/xxx/reproductions/node_modules/.pnpm/esbuild@0.17.18/node_modules/esbuild/lib/main.js:776:5)
    at /xxx/reproductions/node_modules/.pnpm/esbuild@0.17.18/node_modules/esbuild/lib/main.js:2172:68
    at new Promise (<anonymous>)
    at Object.context (/xxx/reproductions/node_modules/.pnpm/esbuild@0.17.18/node_modules/esbuild/lib/main.js:2172:27)
    at Object.context (/xxx/reproductions/node_modules/.pnpm/esbuild@0.17.18/node_modules/esbuild/lib/main.js:2012:58)
    at prepareEsbuildScanner (file:///xxx/reproductions/node_modules/.pnpm/vite@4.3.2/node_modules/vite/dist/node/chunks/dep-7efa13d7.js:43041:26)

  The plugin "vite-plugin-dynamic-import:pre-bundle" was triggered by this import

    main.js:20:27:
      20 │ import rewritePattern from '../bar/node_modules/regexpu-core'
         ╵                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


    at failureErrorWithLog (/xxx/reproductions/node_modules/.pnpm/esbuild@0.17.18/node_modules/esbuild/lib/main.js:1636:15)
    at /xxx/reproductions/node_modules/.pnpm/esbuild@0.17.18/node_modules/esbuild/lib/main.js:1048:25
    at runOnEndCallbacks (/xxx/reproductions/node_modules/.pnpm/esbuild@0.17.18/node_modules/esbuild/lib/main.js:1471:45)
    at buildResponseToResult (/xxx/reproductions/node_modules/.pnpm/esbuild@0.17.18/node_modules/esbuild/lib/main.js:1046:7)
    at /xxx/reproductions/node_modules/.pnpm/esbuild@0.17.18/node_modules/esbuild/lib/main.js:1058:9
    at new Promise (<anonymous>)
    at requestCallbacks.on-end (/xxx/reproductions/node_modules/.pnpm/esbuild@0.17.18/node_modules/esbuild/lib/main.js:1057:54)
    at handleRequest (/xxx/reproductions/node_modules/.pnpm/esbuild@0.17.18/node_modules/esbuild/lib/main.js:723:19)
    at handleIncomingPacket (/xxx/reproductions/node_modules/.pnpm/esbuild@0.17.18/node_modules/esbuild/lib/main.js:745:7)
    at Socket.readFromStdout (/xxx/reproductions/node_modules/.pnpm/esbuild@0.17.18/node_modules/esbuild/lib/main.js:673:7)

Here is the update:
https://github.com/Jinjiang/reproductions/tree/vite-plugin-commonjs-20230502
Jinjiang/reproductions@fb71ab3

[caoxiemeihao]

@Jinjiang 👋 v0.7.1 is out!

[Jinjiang]

It works now. Thanks.