Vite 2.7.0-beta.4 regression when building for SSR targeting Cloudflare Workers

[nrgnrg]

Describe the bug

I'm using Vite to build an SSR app targeting Cloudflare Workers.

Below is my config for the build of the server entry:

  publicDir: false,
  build: {
    minify: true,
    outDir: serverOutDir,
    rollupOptions: {
      input: { index: entry },
      output: {
        output: {
          manualChunks: () => "index.js",
          dir: serverOutDir,
        },
      },
    },
  },
  ssr: {
    target: "webworker",
    noExternal: true,
  },

The intention is to output a single file that runs in a CF Worker and this works in Vite 2.6.x up until 2.7.0-beta.4 where it breaks.

In 2.7.0-beta.4 the build completes fine and it still outputs 1 file as it should but at the end of the generated code it includes export default os();.

This will not run in a Cloudflare Worker as the output is expected to be 1 file and it does not support esm, this causes a SyntaxError: Unexpected token 'export' error in the worker.

Reproduction

n/a

System Info

System:
    OS: macOS 11.4
    CPU: (8) arm64 Apple M1
    Memory: 300.30 MB / 16.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 16.13.0 - ~/.nvm/versions/node/v16.13.0/bin/node
    Yarn: 1.22.10 - /usr/local/bin/yarn
    npm: 8.1.0 - ~/.nvm/versions/node/v16.13.0/bin/npm
  Browsers:
    Chrome: 96.0.4664.55
    Firefox: 94.0.1
    Safari: 14.1.1
  npmPackages:
    vite: 2.7.0-beta.4 => 2.7.0-beta.4

Used Package Manager

pnpm

Logs

No response

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• 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 https://github.com/vuejs/vue-next instead.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
• The provided reproduction is a minimal reproducible example of the bug.

[github-actions]

Hello @nrgnrg. Please provide a minimal reproduction using a GitHub repository or StackBlitz. Issues marked with need reproduction will be closed if they have no activity within 3 days.

[patak-dev]

@nrgnrg could you help us by providing a link to a minimal repro? It would make it easier for other collaborators and maintainers to check this issue

[nrgnrg]

hey @patak-js, repro here: https://stackblitz.com/edit/vitejs-vite-zexect?file=vite-plugin.ts

If you go to dist/server/index.js and scroll right to the end of the file you'll see the issue which is the export default.

If you then delete the dist folder and roll back vite to 2.7.0-beta.3, run another build, export default will not be there and the build will be runnable in a CF worker.

[frandiox]

@patak-js I haven't seen this issue before but I think the problem is related to the minification.
If you remove the minify: true, the export default is gone. In fact, it happens also when you remove all the imports from worker.ts, so it's not related to the app. Did anything related to minification change in the beta?

Apart form that, @nrgnrg you'll want to provide rollupOptions.output.format: 'es' to avoid calling require in a worker, but this only works in Vite >= 2.7.0-beta.8

[haoqunjiang]

Caused by this line:

vite/packages/vite/src/node/plugins/ssrRequireHook.ts

Line 58 in 08a1ec7

parent = module

The reference to module makes esbuild think that the whole chunk is a CommonJS module.
But the output format is esm, so it needs to convert the CommonJS module's module.exports to export default
The CommonJS module's actual exports can only be determined at runtime, so it gets wrapped inside a function call which will return its module.exports at the end, and then esbuild transforms it into export default the_wrapper_function().

[haoqunjiang]

But it's weird, #5714 should have fixed this.

But the actual behavior looks like that ssrRequireHookPlugin is injected when building the client bundle, and gets reused when building the SSR bundle.

Cc @bluwy @patak-js do you have any idea of what happened here? I'm really confused.

[patak-dev]

I don't see a cache that could cause issues if the plugin is reused though. Ping @aleclarson, you may have some ideas here

[bluwy]

Ah yes #5714 should fix it, only if the build.output is es as suggested by @frandiox. So if we configure that and upgrade to beta.8 the issue should be resolved.

I think the question now is whether we want to skip dedupeRequire in worker builds too? (dedupeRequire uses require() which caused the problem). If so we can apply a similar fix for that as #5714.

I've asked @frandiox about this in discord and confirmed that workers are always written in ESM, so an extra skip check isn't needed. But from the issue, Cloudflare Workers seem to not support ESM?

[nrgnrg]

@bluwy Workers by default are written in the service-worker format which is what this build is targeting.

The is now beta support for module syntax but this hasn't been suitable for SSR because the kv-asset-handler doesn't (or didn't) fully support it

[haoqunjiang]

Ah yes, I missed the es part.
I think I now understand where this bug is from.

It's more subtle than I expected.

Here, the nested output.output in your configuration:

completely overrides the output config that Vite generates for the project.

It can override the entire config (instead of merging) because Rollup has this logic https://github.com/rollup/rollup/blob/ca86df280288656c66a948e122c36ccee7e06aca/src/rollup/rollup.ts#L212 that would read output.output instead of output when such field exists.

The override caused the harm because the new config doesn't have a format field, so Rollup uses the default es format. When passing format: 'es' to esbuild for minification, esbuild adds an export default due to the process that I explained in #5812 (comment)

Without the override, Vite uses cjs as the output format for SSR bundles so it works fine.

vite/packages/vite/src/node/build.ts

Line 483 in cf03ecf

format: ssr ? 'cjs' : 'es',

[haoqunjiang]

So the fix in Vite is also quite straightforward: warn or throw here when it detects an output.output field from user configuration:

vite/packages/vite/src/node/build.ts

Line 480 in cf03ecf

const buildOutputOptions = (output: OutputOptions = {}): OutputOptions => {

The API design makes sense in Rollup because it separates rollup.rollup and rollup.generate, where the latter only needs the output configuration, but the user can also provide the same argument to both functions for convenience.

Vite doesn't need such a design.

[bluwy]

I forgot to send my comment before making the PR 😅

---

Thanks for the explanation @nrgnrg. I think the best way forward now is to check noExternal true instead of webworker, so it's more generalized if one day workers decide to support CJS and require stuff. Will make a PR. (Already made)

[bluwy]

Great further investigation @sodatea. Looks like there are two issues here that causes the problem, I agree that having output.output seems to be causing a lot of side effects, I think a warning makes sense to not bring a breaking change.

[nrgnrg]

So the fix is to set rollupOptions.output.format: 'es' in the config? this seems to work for me in the latest beta.

Setting output.output was as far as I'm aware the only way I could make Vite ignore dynamic imports as code splitting points. A worker must be only 1 file but the client build should to be code split so there needs to be a way to force Vite to bundle the worker ignoring dynamic imports, whilst not changing the way it bundles the client code.

If output.output is deprecated, is there a correct way to achieve this?

[haoqunjiang]

format: 'cjs' works too.