Repository navigation
Compute CommonJSModuleIndicator in the parser - #65073
Open
Jake Bailey (jakebailey) wants to merge 1 commit into
Open
Jake Bailey (jakebailey) wants to merge 1 commit into
Jake Bailey (jakebailey) wants to merge 1 commit into
Conversation
Jake Bailey (jakebailey)
requested review from
Andrew Branch (andrewbranch)
and
a balanced review from Copilot
October 31, 2026 06:15
typescript-automation
Bot
added
Author: Team
For Uncommitted Bug
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Two critical reparsing defects leave CommonJS module indicators missing or stale, causing binding panics for valid JavaScript inputs.
2 open issues
What changed in this PR
Moves CommonJS module detection from the binder to the parser so program reuse decisions no longer depend on whether a file has been bound.
Changes:
- Tracks CommonJS constructs during parsing, including reparsed JSDoc nodes.
- Updates the binder to consume parser-owned metadata while preserving symbol creation order.
- Adds tests for module detection and program reuse across CommonJS edits.
| File | Description |
|---|---|
tsc/internal/parser/reparser.go |
Checks cloned JSDoc nodes for CommonJS constructs. |
tsc/internal/parser/parser.go |
Tracks and publishes CommonJS module indicators during parsing. |
tsc/internal/parser/parser_test.go |
Adds CommonJS detection cases. |
tsc/internal/compiler/program_test.go |
Tests reuse across CommonJS edits and script transitions. |
tsc/internal/binder/binder.go |
Uses parser metadata while retaining deferred module symbol creation. |
Review effort: Balanced
Member
Author
|
Uh. Interesting |
CommonJSModuleIndicator was only set by the binder, so any reader of an unbound file saw nil and treated a CommonJS JS file as a script. In particular, canReplaceFileInProgram compares module-ness of the old (usually bound) file against the freshly parsed new file, so editing any CommonJS file always rebuilt the whole program, while a script turning into CommonJS was wrongly reused. The binder also wrote this field on files shared between programs via the parse cache. Whether a file is CommonJS is purely syntactic, so the parser now records it as SourceFile.IsCommonJSModule. Nothing read the indicator node itself, so a bool is enough. Reparsed JSDoc nodes count too, since the binder visits them. The binder no longer writes the field, but still creates the file symbol at the first CommonJS construct to keep symbol creation order, and asserts that it agrees with the parser.
Jake Bailey (jakebailey)
force-pushed
the
cjs-indicator
branch
from
October 31, 2026 12:30
0a43778 to
fce61e8
Compare
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The parser/binder ownership change requires classification parity across JSDoc and top-level-await reparsing while preserving symbol order, but the necessary compiler, binder regression, and race-test validation is unavailable in this review environment.
No open issues 🎉
2 resolved since last review
Review effort: Balanced
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.
CommonJSModuleIndicatoris read on files when seeing if they can be reused. But, that field is only set when a file has been bound! This means that file reuse is racy, but also inconsistent depending on when something was bound.Instead of doing something drastic like forcing a bind just to check reuse, move this info gathering to the parser, which can calculate the same thing earlier.
Funnily, the
SourceFiletype already hadCommonJSModuleIndicatorin the parser-owned field section, which was wrong.Unfortunately,
CommonJSModuleIndicatordepends on JSDoc nodes, so this requires a bit more care. I think it using JSDoc nodes is utterly strange, but, that's what we've always done, even before TS 7.Arguably, we don't even need to set a node for CommonJSModuleIndicator or have one at all, it's not in the public JS API. I might push again to this PR and just make it a bool or tristate or something. Though it not being there but part of the reuse calculation makes me wonder if it should actually be in the AST API after all?