Skip to content

Conversation

@andrewbranch
Copy link
Member

Noticed this while refactoring some code in Corsa.

CopilotAI review requested due to automatic review settings June 4, 2025 17:33
@github-project-automationgithub-project-automationbot moved this to Not started in PR BacklogJun 4, 2025
@typescript-bottypescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jun 4, 2025
Copy link
Contributor

CopilotAI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes the helpers emission for .cjs files when running in ESM module mode by refining the underlying logic and adding new test cases.

  • Adds a new test case (ctsFileInEsnextHelpers.ts) and updates the expected baselines.
  • Adjusts module specifier resolution by removing the fallback to the source file’s implied node format.
  • Refines the condition for emitting external helper imports to better distinguish between CommonJS and ES module formats.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

FileDescription
tests/cases/compiler/ctsFileInEsnextHelpers.tsIntroduces a new test file for .cts files in ESM module mode.
tests/baselines/reference/ctsFileInEsnextHelpers.*Updates baseline outputs (types, symbols, JS, errors) to reflect the new helper emit fix.
src/compiler/moduleSpecifiers.tsRemoves fallback to importingSourceFile.impliedNodeFormat, using overrideImportMode only.
src/compiler/factory/utilities.tsAdjusts the condition for creating external helpers import to avoid CJS emission in ESM mode.
Comments suppressed due to low confidence (1)

src/compiler/moduleSpecifiers.ts:515

  • Removing the fallback to 'importingSourceFile.impliedNodeFormat' might lead to undefined behavior when overrideImportMode is not set; consider explicitly handling the unset case to ensure the module specifier is resolved as intended.
options.overrideImportMode || importingSourceFile.impliedNodeFormat, 

Comment on lines +14 to +15
returntslib_1.__awaiter(this,void0,void0,function(){
returntslib_1.__generator(this,function(_a){
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before the change, emit was missing the tslib_1. on these

impliedModuleKind!==ModuleKind.CommonJS&&
((moduleKind>=ModuleKind.ES2015&&moduleKind<=ModuleKind.ESNext)||
impliedModuleKind===ModuleKind.ESNext||
impliedModuleKind===undefined&&moduleKind===ModuleKind.Preserve)
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to simplify this condition in Corsa since we’re removing System, AMD, and UMD. Opting to make the minimal change here rather than try to clean up.

@github-project-automationgithub-project-automationbot moved this from Not started to Needs merge in PR BacklogJun 5, 2025
@andrewbranchandrewbranch merged commit 7715955 into microsoft:mainJun 6, 2025
32 checks passed
@github-project-automationgithub-project-automationbot moved this from Needs merge to Done in PR BacklogJun 6, 2025
@microsoftmicrosoft locked as resolved and limited conversation to collaborators Dec 9, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: TeamFor Uncommitted BugPR for untriaged, rejected, closed or missing bug

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants

@andrewbranch@weswigham@typescript-bot