Repository navigation
Fix CommonJS declaration emit crashes - #65498
Open
Jake Bailey (jakebailey) wants to merge 3 commits into
Open
Jake Bailey (jakebailey) wants to merge 3 commits into
Jake Bailey (jakebailey) wants to merge 3 commits into
Conversation
typescript-automation
Bot
added
Author: Team
For Uncommitted Bug
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The fallback loses class type bindings and produces circular declarations, while signed-zero handling imports the wrong property for computed numeric keys.
2 open issues
What changed in this PR
This PR addresses CommonJS declaration-emission crashes in the native TypeScript compiler when JavaScript bindings or export names cannot be represented directly as imports or exports.
Changes:
- Falls back to typed declarations for require destructuring with dynamic keys.
- Normalizes numeric and computed import names and template-literal export names.
- Adds three regression tests with declaration, type, and symbol baselines.
| File | Description |
|---|---|
tsc/testdata/tests/cases/compiler/declarationEmitCommonjsTemplateExportName.ts |
Tests template-literal export names. |
tsc/testdata/tests/cases/compiler/declarationEmitCommonjsRequireNumericKey.ts |
Tests numeric and literal computed require keys. |
tsc/testdata/tests/cases/compiler/declarationEmitCommonjsRequireComputedKey.ts |
Tests dynamic keys and mixed destructuring. |
tsc/testdata/baselines/reference/compiler/declarationEmitCommonjsTemplateExportName.types |
Records template-export types. |
tsc/testdata/baselines/reference/compiler/declarationEmitCommonjsTemplateExportName.symbols |
Records template-export symbols. |
tsc/testdata/baselines/reference/compiler/declarationEmitCommonjsTemplateExportName.js |
Records normalized export declarations. |
tsc/testdata/baselines/reference/compiler/declarationEmitCommonjsRequireNumericKey.types |
Records numeric-key binding types. |
tsc/testdata/baselines/reference/compiler/declarationEmitCommonjsRequireNumericKey.symbols |
Records numeric-key binding symbols. |
tsc/testdata/baselines/reference/compiler/declarationEmitCommonjsRequireNumericKey.js |
Records normalized named imports. |
tsc/testdata/baselines/reference/compiler/declarationEmitCommonjsRequireComputedKey.types |
Records mixed-destructuring types. |
tsc/testdata/baselines/reference/compiler/declarationEmitCommonjsRequireComputedKey.symbols |
Records mixed-destructuring symbols. |
tsc/testdata/baselines/reference/compiler/declarationEmitCommonjsRequireComputedKey.js |
Records fallback declarations and sibling import. |
tsc/internal/transformers/declarations/transform.go |
Adds conversion guards and name normalization. |
Review effort: Balanced
Jake Bailey (jakebailey)
force-pushed
the
compiler-fixes/commonjs-declaration-names
branch
from
November 14, 2026 05:24
f6ffd04 to
fda0478
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The partial-import transformation bypasses visibility and stripInternal checks, potentially emitting unused imports from unpublished private modules and causing module-resolution errors for consumers.
1 open issue
2 resolved since last review
Review effort: Balanced
Declaration emit for CommonJS files can build import and export specifiers whose names are not valid module export names: destructured require bindings with numeric or computed keys, and exports named by template literals that are keywords or not identifiers. The printer rejects these names and panics, so these cases crash rather than produce a baseline. Add the cases ahead of the fixes; their baselines are added with the fixes.
Declaration emit turns a destructured CommonJS require into a named import, passing each binding's property name straight through as the import name. Numeric keys and computed keys are not valid module export names, so the printer panicked on the resulting import specifiers. Normalize numeric and literal computed keys to an identifier or a string literal, using the string form of signed numeric keys so that [-0] names property "0". Keys that are truly dynamic cannot be expressed as an import, so declare those bindings, along with any nested or rest bindings in the same pattern, as variables with their types. The remaining bindings are still imported, which keeps names such as an imported class usable as types.
A CommonJS export named by a template literal, such as exports[`class`] or Object.defineProperty(exports, `a b`, ...), kept its template literal name when that name was a keyword or not an identifier. Template literals are not valid module export names, so the printer panicked on the resulting export specifier, and duplicate exports of the same name were not detected. Convert any remaining template literal name to a string literal.
Jake Bailey (jakebailey)
force-pushed
the
compiler-fixes/commonjs-declaration-names
branch
from
November 14, 2026 05:42
fda0478 to
cf81125
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These crash because we can't covert the require to an import; fix them.