More module specifier generation #1051
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors how module specifiers and output paths are generated by replacing the SourceFileMetaDataProvider dependency with a getEmitModuleFormatOfFile function and updating related transformer, compiler, and modulespecifiers APIs to use the revamped HasFileName interface and FileName field. Key changes include:
- Replacing and updating transformer constructors (ESModuleTransformer and CommonJSModuleTransformer) to rely on the getEmitModuleFormatOfFile callback.
- Updating module specifier generation to use the new FileName field in ModulePath and modifying several APIs throughout the compiler and printer packages.
- Removing legacy interfaces and helper functions (e.g. SourceFileMetaDataProvider) while integrating new output path functionality.
Reviewed Changes
Copilot reviewed 308 out of 308 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/transformers/* | Refactored transformer constructors and adjusted API parameters to use getEmitModuleFormatOfFile. |
| internal/modulespecifiers/* | Updated ModulePath to use FileName instead of Path and revised related specifier logic. |
| internal/compiler/, internal/printer/, internal/ast/* | Updated API signatures to work with the new HasFileName interface and outputpaths package. |
| internal/checker/checker.go | Modified resolved module checks; potential nil-check removal may cause runtime issues. |
| internal/outputpaths/* | Added and integrated new output paths functions to support the refactor. |
| ->Q : typeof import("sub") | ||
| +>Q : typeof import("./sub.js") | ||
| +>Q : typeof import("./sub") |
There was a problem hiding this comment.
I think this is something that @weswigham mentioned, that type baselines in Strada did not use real module specifier generation, but they (intentionally?) do in Corsa. That significantly limits the number of diffs that will get completely deleted. IMO, it would be nice to align them and see a pure diff deletion, whether by changing Corsa or changing Strada now that we have a target tsgo-port branch.
There was a problem hiding this comment.
Do you know where that might be? Happy to fix that and push it up.
There was a problem hiding this comment.
The typeToTypeNode call in typeWriter.ts provides no symbol tracker, and thus no module resolution host. That's fine, but in such a case, the createBasicNodeBuilderModuleSpecifierResolutionHost call inside checker that makes a fallback only partially wraps the host program, and only when InternalNodeBuilderFlags.DoNotIncludeSymbolChain is passed (which, it is not, in this case). Make that fallback unconditional, and make the wrapper more complete, and you should match the new Corsa behavior.
|
|
||
| func (tx *ImpliedModuleTransformer) getEmitModuleFormatOfFile(node *ast.SourceFile) core.ModuleKind { | ||
| // !!! host.getEmitModuleFormatOfFile? | ||
| return ast.GetEmitModuleFormatOfFileWorker(node, tx.compilerOptions, tx.sourceFileMetaDataProvider.GetSourceFileMetaData(node.Path())) |
There was a problem hiding this comment.
This (and similar dupes) really bugged me when I was trying to do the module indicator 👍
| for _, modulePath := range modulePaths { | ||
| targetPath := tspath.ToPath(modulePath.FileName, host.GetCurrentDirectory(), info.UseCaseSensitiveFileNames) | ||
| var existingImport *ast.StringLiteralLike | ||
| for _, importSpecifier := range importingSourceFile.Imports() { |
There was a problem hiding this comment.
In Strada, this code uses program file include reasons instead of file.imports, which I found very puzzling. I changed it in Strada and the only tests that fail were ones that use --outFile. (The existing imports it finds are synthesized ambient module imports inside ambient module declarations in the generated .d.ts bundle. Since those are attached to module declaration blocks instead of a source file, importingSourceFile.Imports() doesn’t find them. This code is supposed to be an optimization, so missing that edge case seems fine.)
There was a problem hiding this comment.
I dunno. I'd have guessed something like "so it works for files which weren't assembled by the default program, since .imports is set by the program and not the parser", but in such a scenario, I kinda doubt the host would still provide file include reasons. I guess it could happen...?
Follow-up to #851 and #791 eliminating more diffs