Fix auto-imports inserting import in CJS files when moduleDetection is force#3328
Fix auto-imports inserting import in CJS files when moduleDetection is force#3328
import in CJS files when moduleDetection is force#3328Conversation
… module kinds When using module: "node16"/"node18"/"node20"/"nodenext", moduleDetection defaults to "force", which sets ExternalModuleIndicator on ALL files regardless of actual syntax. This made the file content indicators unreliable for determining CJS vs ESM, and the simple module kind comparison (GetEmitModuleKind() < ModuleKindES2015) also failed because Node module kinds have numeric values >= 100. The fix adds an early check for Node module kinds that uses GetImpliedNodeFormatForEmit(), which correctly determines the runtime module format based on file extension and package.json "type" field. Agent-Logs-Url: https://github.com/microsoft/typescript-go/sessions/9405eb29-fef7-4989-a974-aba9130e0d67 Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
import in CJS files under node16/node20/nodenext
andrewbranch
left a comment
andrewbranch
left a comment
There was a problem hiding this comment.
Any time --moduleDetection is force (explicitly or implicitly) we shouldn't be using ExternalModuleIndicator. The real problem scenario is this:
--module preserve--moduleDetection force- no package.json
type - JS file that only uses
requireso far
The intent of looking at ExternalModuleIndicator and CommonJSModuleIndicator is to use existing syntax file as a strong indicator of what should come next. If --moduleDetection force makes that approach unusable, we need a different detection mechanism for existing syntax, because in the scenario I just described, I can't think of a more reliable signal outside of existing syntax.
You should make a utility function that uses ExternalModuleIndicator and CommonJSModuleIndicator if possible, and if not, achieves the same thing by inspecting the source file's .Imports(). Don't forget to ignore synthesized imports.
Instead of special-casing Node module kinds, create a general-purpose detectSyntax utility that handles moduleDetection "force" properly. When moduleDetection is NOT "force", ExternalModuleIndicator and CommonJSModuleIndicator are reliable indicators of the file's syntax. When moduleDetection IS "force", ExternalModuleIndicator may be set to the source file node itself (not a genuine syntax indicator), so we: 1. Check if ExternalModuleIndicator points to a real statement node 2. Fall back to scanning Imports() for actual import/export declarations 3. Skip synthesized imports This handles the broader problem: any time moduleDetection is "force" (explicitly or via Node module kinds), the syntax-based detection works correctly. Also added a test for --module preserve + --moduleDetection force with a JS file containing only require() syntax. Agent-Logs-Url: https://github.com/microsoft/typescript-go/sessions/16d8ed48-1daa-42b8-a8b2-c20032f4bbdb Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
Agent-Logs-Url: https://github.com/microsoft/typescript-go/sessions/16d8ed48-1daa-42b8-a8b2-c20032f4bbdb Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
Implemented in 8e33931. Created
Also applied the same utility to the "other files" fallback (step 5), and kept the Added a third test for |
import in CJS files under node16/node20/nodenextimport in CJS files when moduleDetection is force
| if core.ModuleKindNode16 <= moduleKind && moduleKind <= core.ModuleKindNodeNext { | ||
| return v.program.GetImpliedNodeFormatForEmit(v.importingFile) != core.ModuleKindESNext | ||
| } |
There was a problem hiding this comment.
@copilot Instead of adding this new condition within here, I think you can just swap the order of 3 and 4
There was a problem hiding this comment.
Done in 0661560. Swapped the order so GetImpliedNodeFormatForEmit is checked first (step 3) and the config file module kind check is now step 4, removing the special Node module kind condition entirely.
…seRequire Move GetImpliedNodeFormatForEmit check (step 3) before the config file module kind check (step 4). This removes the need for the special Node module kind condition since GetImpliedNodeFormatForEmit already handles Node module kinds correctly via file extension and package.json "type". Agent-Logs-Url: https://github.com/microsoft/typescript-go/sessions/7983d9fc-3c93-4331-b786-935d5355882e Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
With
moduleDetection: "force"(explicitly or implicitly via Node module kinds likenode16/node18/node20/nodenext), auto-imports incorrectly emitimportstatements in CJS files instead ofrequire()calls.The root problem is that
--moduleDetection forcesetsExternalModuleIndicatorto the source file node itself on all files unconditionally, making the CJS/ESM content heuristic incomputeShouldUseRequire()unable to distinguish CJS files from ESM files. A secondary issue is thatGetEmitModuleKind() < ModuleKindES2015returnsfalsefor Node module kinds (ModuleKindNode20 = 102) even though the file may be CJS.Fix
Created
detectSyntax/detectSyntaxIndicatorsutility functions that properly detect ESM vs CJS syntax regardless ofmoduleDetectionmode:moduleDetectionis not"force": usesExternalModuleIndicatorandCommonJSModuleIndicatordirectly (they are reliable)moduleDetectionis"force": checks ifExternalModuleIndicatorpoints to a genuine statement node (notfile.AsNode()), and falls back to scanning the source file'sImports()for actual import/export declarations, skipping synthesized importsCommonJSModuleIndicatoris always reliable regardless ofmoduleDetectionmode, since it is set by the binder based on actualrequire()/module.exportssyntax.Also reordered the fallback steps in
computeShouldUseRequireso thatGetImpliedNodeFormatForEmit()is checked before the config file's module kind. This naturally handles Node module kinds (which determine the runtime format from file extension andpackage.json"type") without needing a special case.Tests
Three new fourslash tests:
module.exportssyntax +module: "node20"+package.json"type": "commonjs"ExternalModuleIndicatoralone causes the wrong result--module preserve+--moduleDetection force+ JS file with onlyrequire()syntax and nopackage.json"type"