-
Notifications
You must be signed in to change notification settings - Fork 658
Port more resolveExternalModule, fix copy paste error in GetResolutionDiagnostic #1248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this 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 centralizes module resolution diagnostics into GetResolutionDiagnostic
, removes a duplicate implementation in fileloader.go
, and updates error reporting in the checker to emit extension-related diagnostics correctly.
- Introduces
GetResolutionDiagnostic
ininternal/module/util.go
to handle JSX, JS, JSON, and arbitrary-extension rules. - Refactors
fileloader.go
to call the new centralized diagnostic function and removes the old local copy. - Enhances
checker.go
to capture and report the new diagnostics early in external module resolution.
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated no comments.
File | Description |
---|---|
internal/module/util.go | New GetResolutionDiagnostic function for extension-based errors. |
internal/compiler/fileloader.go | Replaced local diagnostic call with module.GetResolutionDiagnostic and removed duplicate logic. |
internal/checker/checker.go | Added resolutionDiagnostic handling and updated source-file/error logic, plus helper resolutionExtensionIsTSOrJson . |
Comments suppressed due to low confidence (4)
internal/module/util.go:135
- New branches in
GetResolutionDiagnostic
(especially the default case for arbitrary extensions) should have dedicated unit tests to ensure correct diagnostics are emitted for each extension type.
switch resolvedModule.Extension {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll admit I don't know how/why this fixes the error in VS Code (maybe you can comment on why), but the implementation looks like it matches the existing Strada behavior.
c.error(errorNode, resolutionDiagnostic, moduleReference, resolvedModule.ResolvedFileName) | ||
} else { | ||
isExtensionlessRelativePathImport := tspath.PathIsRelative(moduleReference) && !tspath.HasExtension(moduleReference) | ||
resolutionIsNode16OrNext := c.moduleResolutionKind == core.ModuleResolutionKindNode16 || c.moduleResolutionKind == core.ModuleResolutionKindNodeNext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in the current snapshot, but does this not kick in for node20
in Strada?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the description on microsoft/TypeScript#61805; it would if Corsa had node20.
The VS Code problem is specifically that a side effect import was being resolved to a JS file (lacking type definitions), but we weren't handling that case. |
This fixes this error from the VS Code repo:
Among other problems.