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

vite6 [commonjs--resolver] node_modules/testb/a.js (1:6): await isn't allowed in non-async function #18850

Closed
7 tasks done
zhangHongEn opened this issue Dec 1, 2024 · 16 comments

Comments

@zhangHongEn
Copy link

zhangHongEn commented Dec 1, 2024

Describe the bug

Everything works fine with Vite 5.

Top-level await in node_modules.
module-federation/vite#201

Reproduction

https://github.com/zhangHongEn/vite-mf-bug

Steps to reproduce

npm install && npm run build

System Info

System:
    OS: macOS 14.3.1
    CPU: (8) arm64 Apple M2
    Memory: 2.71 GB / 24.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 18.20.2 - ~/.nvm/versions/node/v18.20.2/bin/node
    Yarn: 1.22.22 - ~/.nvm/versions/node/v18.20.2/bin/yarn
    npm: 10.5.0 - ~/.nvm/versions/node/v18.20.2/bin/npm
    pnpm: 9.6.0 - ~/.nvm/versions/node/v18.20.2/bin/pnpm
  Browsers:
    Safari: 17.3.1

Used Package Manager

npm

Logs

> vite-project@0.0.0 build
> vite build

vite v6.0.1 building for production...
✓ 9 modules transformed.
x Build failed in 104ms
error during build:
[commonjs--resolver] node_modules/.pnpm/test-top-level-await@1.0.0/node_modules/test-top-level-await/a.js (1:6): await isn't allowed in non-async function
file: /Users/zhanghongen/Desktop/zwork/open-code/testdir/vite-project/node_modules/.pnpm/test-top-level-await@1.0.0/node_modules/test-top-level-await/a.js:1:6

1: await 1
         ^
2: exports = {}

    at getRollupError (file:///Users/zhanghongen/Desktop/zwork/open-code/testdir/vite-project/node_modules/.pnpm/rollup@4.28.0/node_modules/rollup/dist/es/shared/parseAst.js:396:41)
    at ParseError.initialise (file:///Users/zhanghongen/Desktop/zwork/open-code/testdir/vite-project/node_modules/.pnpm/rollup@4.28.0/node_modules/rollup/dist/es/shared/node-entry.js:13286:28)
    at convertNode (file:///Users/zhanghongen/Desktop/zwork/open-code/testdir/vite-project/node_modules/.pnpm/rollup@4.28.0/node_modules/rollup/dist/es/shared/node-entry.js:14997:10)
    at convertProgram (file:///Users/zhanghongen/Desktop/zwork/open-code/testdir/vite-project/node_modules/.pnpm/rollup@4.28.0/node_modules/rollup/dist/es/shared/node-entry.js:14240:12)
    at Module.setSource (file:///Users/zhanghongen/Desktop/zwork/open-code/testdir/vite-project/node_modules/.pnpm/rollup@4.28.0/node_modules/rollup/dist/es/shared/node-entry.js:15983:24)
    at async ModuleLoader.addModuleSource (file:///Users/zhanghongen/Desktop/zwork/open-code/testdir/vite-project/node_modules/.pnpm/rollup@4.28.0/node_modules/rollup/dist/es/shared/node-entry.js:19879:13)

Validations

@zhangHongEn zhangHongEn changed the title [commonjs--resolver] node_modules/testb/a.js (1:6): await isn't allowed in non-async function vite6 [commonjs--resolver] node_modules/testb/a.js (1:6): await isn't allowed in non-async function Dec 1, 2024
@sapphi-red
Copy link
Member

It doesn't work with Vite v5 too.
https://stackblitz.com/edit/github-441yb3?file=package.json&terminal=build

I don't think this should work because top-level await cannot be used in CJS (nodejs/node#21267).

@zhangHongEn
Copy link
Author

zhangHongEn commented Dec 2, 2024 via email

@sapphi-red
Copy link
Member

sapphi-red commented Dec 2, 2024

Yes, I have.

@zhangHongEn
Copy link
Author

zhangHongEn commented Dec 2, 2024 via email

@zhangHongEn
Copy link
Author

My code is very simple. I think as long as Vite can skip the checks, vite-plugin-top-level-await can handle it.

const remoteReact = await moduleFederation.import('react', "^18.0.1")
module.exports = remoteReact

@zhangHongEn
Copy link
Author

My code is very simple. I think as long as Vite can skip the checks, vite-plugin-top-level-await can handle it.

const remoteReact = await moduleFederation.import('react', "^18.0.1")
module.exports = remoteReact

However, if there are other ways to implement this runtime module, I’m more than happy to make changes.

@sapphi-red
Copy link
Member

It seems setting build.commonjsOptions.strictRequires: 'auto' works. (Note that test-top-level-await/a.js should include module.exports instead of exports, otherwise the CJS conversion should not run).

When strictRequires is true (which is a safer value and Vite 6 now uses that), the commonjs plugin wraps the module with IIFE and the plugin doesn't expect await to be used in the module. Because the IIFE is not a async function, the error happens.

Setting build.commonjsOptions.strictRequires: 'auto' would make the build work. But note that setting that option may cause non-deterministic builds (rollup/plugins#1425).

Instead of relying on the CJS plugin, you might be able to use the syntheticNamedExports option.
https://rollupjs.org/plugin-development/#synthetic-named-exports

@zhangHongEn
Copy link
Author

Can syntheticNamedExports be used in dev mode?

@sapphi-red
Copy link
Member

No, syntheticNamedExports won't work in dev.

@zhangHongEn
Copy link
Author

I previously used syntheticNamedExports, but I removed it to maintain consistency between the dev and build processes.

@sapphi-red
Copy link
Member

sapphi-red commented Dec 16, 2024

I'm not sure why your code worked in dev. esbuild doesn't support top-level-await in CJS as well.
https://esbuild.github.io/try/#YgAwLjI0LjAALS1mb3JtYXQ9ZXNtAGUAZm9vLmNqcwBjb25zdCByZW1vdGVSZWFjdCA9IGF3YWl0IDEKbW9kdWxlLmV4cG9ydHMgPSByZW1vdGVSZWFjdA
Do you run commonjs plugin in dev as well?

@zhangHongEn
Copy link
Author

I used to do it this way:
Dev:
export default runtimeModule + optimizeDeps.needsInterop
Build:
syntheticNamedExports

But later, for consistency and simplicity, I changed it to:
module.exports = await runtimeModule

@zhangHongEn
Copy link
Author

zhangHongEn commented Dec 16, 2024

@sapphi-red
Copy link
Member

I see. I don't have any ideas other than (a) going back to optimizeDeps.needsInterop/syntheticNamedExports or (b) set build.commonjsOptions.strictRequires: 'auto' and hope that the builds are stable.

@zhangHongEn
Copy link
Author

ok thanks

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

No branches or pull requests

2 participants