Skip to content

fix(legacy): minify SystemJS legacy chunks with module: false - #23324

Closed
santhiprakash wants to merge 3 commits into
vitejs:mainfrom
santhiprakash:fix/legacy-systemjs-minify
Closed

santhiprakash wants to merge 3 commits into
vitejs:mainfrom
santhiprakash:fix/legacy-systemjs-minify

Conversation

@santhiprakash

Copy link
Copy Markdown
Contributor

What is this PR solving?

Fixes #23296

When using the oxc minifier (Vite 8 default, introduced by #22468), legacy chunks are minified with module: true because rolldown derives this flag from output.format (which remains esm at minify time). The legacy plugin then converts chunks from ESM to SystemJS in renderChunk, which runs after minification.

SystemJS output runs in sloppy mode (no "use strict"), where Annex B.3.3 hoists block-level function declarations to the enclosing function scope. With module: true, the minifier applies strict-mode semantics and can dead-code-eliminate block-scoped functions that are actually reachable via sloppy-mode hoisting. After mangling, this causes name collisions that produce TypeErrors and white screens on legacy browsers.

What changes did you make?

  • packages/plugin-legacy/src/index.ts: Disable rolldown's built-in minification for legacy output chunks (it always uses module: true). Instead, invoke minifySync from rolldown/experimental directly in renderChunk with module: false, compress: { target: 'es2015' }, and mangle: true. This ensures the minifier uses sloppy-mode semantics for SystemJS output. Resolves rolldown through vite's dependency tree since it's not a direct dependency of plugin-legacy.

  • packages/plugin-legacy/src/__tests__/minify-systemjs.spec.ts: New unit tests verifying that minifySync with module: true eliminates block-scoped functions (strict-mode DCE) while module: false preserves them (sloppy-mode safe), and that compress: { target: 'es2015' } doesn't introduce newer syntax.

  • playground/legacy/__tests__/legacy.spec.ts: Updated the minification heuristic from terser-specific patterns to SystemJS format detection, and added a polyfills-legacy assertion.

Alternatives explored

  • "use strict" injection (closed-unmerged PR fix(legacy): emit legacy chunks as strict mode code #23297): Rejected by maintainer sapphi-red as incomplete — the directive alone doesn't fix the DCE/mangling behavior.
  • Output-level minify: { module: false }: Not possible by type — rolldown's OutputOptions.MinifyOptions omits the module field; it's derived internally from output.format.
  • minify: 'dce-only': Also uses ESM module semantics internally, so it has the same DCE problem. Must use false (no built-in minification).

Testing

  • 3 new unit tests in minify-systemjs.spec.ts pass (direct minifySync behavior)
  • All 30 legacy playground e2e tests pass (including updated minification heuristic)
  • All 34 plugin-legacy unit tests pass

The new sloppy-hoisting playground test fixture from earlier iterations was removed because the block-scoped function pattern gets eliminated by rolldown's bundler-phase tree-shaking (not just minification), making it unsuitable for e2e. The unit tests cover the minify-level fix directly.

AI disclosure

This PR was developed with AI assistance. I reviewed all generated code, verified each change against the root cause analysis, ran all tests, and validated the fix approach against the maintainer's guidance (sapphi-red's feedback on #23297).

When using the oxc minifier (Vite 8 default), legacy chunks are minified
with `module: true` because rolldown derives this flag from
`output.format` (which is `esm` at minify time). The legacy plugin then
converts chunks from ESM to SystemJS in `renderChunk`, which runs AFTER
minification. SystemJS output runs in sloppy mode (no "use strict"),
where Annex B.3.3 hoists block-level function declarations to the
enclosing function scope. With `module: true`, the minifier applies
strict-mode semantics and can dead-code-eliminate block-scoped
functions that are actually reachable via sloppy-mode hoisting. After
mangling, this can cause name collisions that produce TypeErrors and
white screens on legacy browsers.

The fix disables rolldown's built-in minification for legacy output
chunks and instead invokes `minifySync` from `rolldown/experimental`
directly in `renderChunk` with `module: false`, ensuring the minifier
uses sloppy-mode semantics for the SystemJS output.

Adds a unit test that directly verifies `minifySync` with `module: true`
eliminates block-scoped functions (strict-mode DCE) while `module: false`
preserves them (sloppy-mode safe). Updates the e2e minification test
heuristic from terser-specific patterns to SystemJS format detection.

fixes #23296
- Escape the literal  inside the System.register regex pattern so the
  regexp/strict ESLint rule accepts it (uses [^\]]* inside a char class).
- Convert the rolldown/experimental resolved path to a file:// URL via
  pathToFileURL before dynamic import, so it works on Windows where bare
  drive-letter paths (D:\\...\\...) are not accepted by the ESM loader
  and fail with 'Received protocol d:'.
@santhiprakash
santhiprakash force-pushed the fix/legacy-systemjs-minify branch from 9799a3e to 7982b9a Compare August 22, 2026 17:33
@santhiprakash

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (a98c8d95d) to clear the merge conflict — only conflict was in packages/plugin-legacy/src/index.ts import block (upstream mainline moved @babel/core/browserslist into the alphabetical stdlib block; kept my added pathToFileURL named import + functional changes intact). playground/legacy/__tests__/legacy.spec.ts auto-merged. New head: 7982b9a9. CI rerun in progress.

The new minify-systemjs.spec.ts introduced in this branch put the
'alphabetically later' vitest import before the rolldown/experimental
import. The CI Lint job's 'pnpm format && git diff --exit-code' step
fails because oxfmt (sortImports enabled in .oxfmtrc.json) wants them
reordered. Swap the two lines so the lint gate can pass on the rebased
head.

No code or test logic changes - pure formatting.
@santhiprakash

Copy link
Copy Markdown
Contributor Author

Follow-up fix on top of 7982b9a9: the rebased head failed the CI Lint job (pnpm format && git diff --exit-code) because .oxfmtrc.json enables sortImports and the new minify-systemjs.spec.ts had the vitest import before rolldown/experimental. Swapped the two lines in packages/plugin-legacy/src/__tests__/minify-systemjs.spec.ts so rolldown/experimental comes first. Pure formatting, no logic changes. New head: 88bbf40f. Lint/Build&Test on Linux/macOS pass; the only remaining failure is a Windows Test serve flake in playground/environment-react-ssr > pre-bundling > deps reload (timing-sensitive test that fails on the Windows runner on mainline too — unrelated to the plugin-legacy changes).

@santhiprakash santhiprakash reopened this Aug 23, 2026
@github-actions github-actions Bot added the bot: likely Likely a bot, LLM, or agent. Automatically comments and closes the issue or PR label Aug 23, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This PR has been automatically flagged as likely to be created by a bot, LLM, or agent, and will be automatically closed. These contributions harm the maintenance of the project. Please read our AI policy for more information.
If you believe this is a mistake, please reply to this comment and we will review it.

@github-actions github-actions Bot closed this Aug 23, 2026
@santhiprakash

santhiprakash commented Aug 23, 2026 •

Copy link
Copy Markdown
Contributor Author

Hi, the close/reopen above was a manual CI retrigger after the Windows Build&Test job flaked, because the santhiprakash account cannot rerun only the failed job on vitejs/vite. The branch and diff are unchanged. Could a maintainer please add the bot: skip label and reopen so the actual CI status can be evaluated? I can explain the legacy-minify diff and respond to review feedback.

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

Labels

bot: likely Likely a bot, LLM, or agent. Automatically comments and closes the issue or PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

legacy: minified as ESM while the output is script (SystemJS)

1 participant