Skip to content

fix(commonjs): treat a require in a catch clause as guarded - #2018

Open
Stadly wants to merge 1 commit into
rollup:masterfrom
Stadly:fix/commonjs-require-in-catch-clause
Open

fix(commonjs): treat a require in a catch clause as guarded#2018
Stadly wants to merge 1 commit into
rollup:masterfrom
Stadly:fix/commonjs-require-in-catch-clause

Conversation

@Stadly

@Stadly Stadly commented Aug 5, 2026

Copy link
Copy Markdown

Rollup Plugin Name: commonjs

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Surfaced via sveltejs/kit#16653 (adapter-node produced an unloadable server bundle because of this).

Description

ignoreTryCatch defaults to true precisely so that a require of a possibly-uninstalled external is left alone instead of being hoisted into a top-level import. But the guard window is computed from the try block alone:

case 'TryStatement':
  if (currentTryBlockEnd === null) {
    currentTryBlockEnd = node.block.end;   // ← try block only
  }

A require in the catch clause starts after node.block.end, so isInsideTryBlock is false for it and it is converted like any unguarded require.

That inverts the intent of the option. The emitted import is evaluated before any of the surrounding code runs, so a failure the author explicitly chose to handle becomes an unconditional one — and when the specifier is unresolvable, the module can never be loaded at all, even though the guarded branch may never execute.

Real-world case

@babel/core's .cts config loader has this shape:

try {
  return m._compile(transformFileSync(filename, opts).code, filename);
} catch (error) {
  const packageJson = require('@babel/preset-typescript/package.json');
  if (semver.lt(packageJson.version, '7.21.4')) { /* warn */ }
  throw error;
}

@babel/preset-typescript is neither a dependency nor a peer of @babel/core — it is a soft lookup that is legitimately absent, which is why the author guarded it. Today it becomes:

import '@babel/preset-typescript/package.json';

at the top of the output. Tree-shaking removes the surrounding Babel code but keeps the dangling side-effect import, and any bundle carrying it dies on evaluation with ERR_MODULE_NOT_FOUND. This took down a production deployment; the build succeeded and the server exited at startup.

Note the sibling require('@babel/preset-typescript') a few lines away — in the try block — is already left alone today. Only the catch one leaks, which is a fairly direct sign the window is the bug rather than the policy.

What changed

Track the end of the whole TryStatement alongside the end of its block, and use that for the ignoreTryCatch decision:

-        isInsideTryBlock && isWrappedId(resolvedId, EXTERNAL_SUFFIX)
+        isInsideTryStatement && isWrappedId(resolvedId, EXTERNAL_SUFFIX)

currentTryBlockEnd is deliberately not widened in place: it also feeds the toBeRemoved comparison against currentConditionalNodeEnd, and changing it there would alter which nodes get removed. A separate currentTryStatementEnd keeps the two concerns independent. isInsideTryBlock had no other reader, so it is replaced rather than duplicated.

finally clauses are covered by the same window, which seems consistent — a require there is equally not something the plugin should hoist out.

Tests

  • New fixture test/fixtures/function/try-catch-handler, requiring an uninstalled external from inside a catch clause on a branch that never runs.
  • It fails on main with Cannot find module 'uninstalled-external-module' — the hoisted import breaking evaluation — and passes with this change.
  • Full suite: 292 passing (291 before, plus the new fixture). The snapshot diff is purely additive; no existing snapshot changed, which is the clearest evidence this doesn't alter behaviour for requires in try blocks or elsewhere.

I also tried a broader variant — treating any unresolvable conditional require this way — and discarded it: it dropped require('node:crypto') and broke 2 tests, because "unresolvable at build time" is not the same as "unavailable at runtime". The narrow change above is the one that holds up.

Docs

Updated the ignoreTryCatch section to say the option covers the whole try-catch statement, including the catch and finally clauses.

`ignoreTryCatch` defaults to `true` so that a `require` of a possibly-uninstalled external is left alone rather than hoisted into a top-level import. The guard window was computed from the try block alone (`node.block.end`), so a `require` in the `catch` clause fell outside it and was converted anyway.

That turns a failure the author chose to handle into an unconditional one: the emitted `import` is evaluated before any of the surrounding code runs, so the module cannot be loaded at all even when the guarded branch would never execute. When the specifier is unresolvable the import also has no chance of ever succeeding, so the whole bundle becomes unloadable.

Track the end of the whole `TryStatement` separately from the end of its block, and use that for the `ignoreTryCatch` decision. `currentTryBlockEnd` is deliberately left as it was, because it also feeds the `toBeRemoved` comparison against `currentConditionalNodeEnd`; widening it in place would have changed which nodes get removed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant