Don't report TS1293 for destructured require under --module preserve - #4800
Open
AMR5210 wants to merge 2 commits into
Open
Don't report TS1293 for destructured require under --module preserve#4800AMR5210 wants to merge 2 commits into
AMR5210 wants to merge 2 commits into
Conversation
Records current behavior: `const { readFile } = require("./dep.cjs")` in a
.cjs file reports TS1293 even though it is CommonJS, not ESM syntax.
AliasDeclarationNode includes BindingElementOfBareOrAccessedRequire, so
`const { readFile } = require("./dep.cjs")` reached the check for ESM
syntax in CommonJS files. Exclude binding elements alongside the
`import =` and plain `const x = require(...)` forms that were already
exempt.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes false TS1293 diagnostics for destructured CommonJS require calls under --module preserve.
Changes:
- Exempts
BindingElementaliases from the ESM syntax check. - Adds regression coverage while retaining diagnostics for genuine ESM imports.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
internal/checker/checker.go |
Exempts destructured require bindings. |
testdata/tests/cases/compiler/modulePreserveRequireDestructuring.ts |
Adds regression and control cases. |
testdata/baselines/reference/compiler/modulePreserveRequireDestructuring.errors.txt |
Verifies only the ESM import errors. |
testdata/baselines/reference/compiler/modulePreserveRequireDestructuring.symbols |
Records expected symbols. |
testdata/baselines/reference/compiler/modulePreserveRequireDestructuring.types |
Records expected inferred types. |
Author
|
@microsoft-github-policy-service agree |
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.
Fixes microsoft/TypeScript#63696
Analysis
In a
.cjsfile checked withcheckJs, a destructuredrequireis reported as ESM syntax:checkAliasSymbolruns for every alias declaration, and three members ofAliasDeclarationNodeare CommonJSrequireforms rather than ESM syntax:ImportEqualsDeclaration:import fs = require("node:fs")VariableDeclarationInitializedTo<RequireOrImportCall | AccessExpression>:const fs = require("node:fs")BindingElementOfBareOrAccessedRequire:const { readFile } = require("node:fs")The
--module preservebranch excluded the first two but not the third, so adestructured
requirewas treated as ESM. This is also why the error isreported on the binding name rather than on the statement.
The check was introduced upstream in microsoft/TypeScript#58825 (TypeScript 5.6,
matching the reported 5.5 to 5.6 regression) and ported here as-is. It is still
present in 5.9, so per CONTRIBUTING it is fixed here rather than in Strada.
Fix
Exclude binding elements from the check, alongside the two
requireforms thatwere already exempt.
I also considered adding
!ast.IsInJSFile(node)to mirror theverbatimModuleSyntaxbranch directly above, but rejected it: this is the onlysite that reports TS1293, and ESM imports in a
.cjsfile are reported throughtheir
ImportSpecifier/ImportClausealiases, so exempting JS files wholesalewould stop flagging genuine ESM syntax in CommonJS JavaScript. Within
AliasDeclarationNode, aBindingElementcan only ever be arequiredestructure, so excluding it is the narrow fix.
The new test covers the destructured form, the plain form as a control, and, in
a separate file, a real ESM import in a
.cjsfile, which must still error. Thebaseline keeps that error, demonstrating the fix does not over-broaden.
Copilot Checklist
I successfully ran these commands at the end of my session, and they completed without error:
AI assistance disclosure: This change was authored with assistance from
Claude Code. I have reviewed and understand the resulting changes and will
respond to review feedback.