Add content mapper output extensions - #64581
Andrew Branch (andrewbranch) wants to merge 9 commits into
Conversation
Preserve source phase import diagnostic codes and renumber content mapper output diagnostics; regenerate outputs and update the baseline. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Preserve content mapper output extensions in the relocated VS Code extension API. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adapt content mapper rewrites, output paths, hosts, and regression tests to strongly typed paths and case sensitivity. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| rewrites := b.ctx.host.ContentMapperExtensionRewrites() | ||
| ignoreCase := b.ctx.host.CaseSensitivity().IsCaseInsensitive() | ||
| if core.ShouldRewriteModuleSpecifierWithExtensions(specifier.AsString(), b.ch.compilerOptions, rewrites, ignoreCase) { | ||
| rewritten, _ := core.RewriteExtension(specifier.AsString(), rewrites, ignoreCase) |
There was a problem hiding this comment.
Should this be a method on something?
Jake Bailey (jakebailey)
left a comment
There was a problem hiding this comment.
I think the casing stuff might be out of date after the typed paths PR?
| EmitResolver: emitResolver, | ||
| GetEmitModuleFormatOfFile: host.GetEmitModuleFormatOfFile, | ||
| ContentMapperExtensionRewrites: host.ContentMapperExtensionRewrites(), | ||
| IgnoreCase: host.CaseSensitivity().IsCaseInsensitive(), |
There was a problem hiding this comment.
Why not just pass the enum value?
| Target string | ||
| } | ||
|
|
||
| func GetExtensionRewrite(path string, rewrites []ExtensionRewrite, ignoreCase bool) (ExtensionRewrite, bool) { |
There was a problem hiding this comment.
Do these belong in tspath, as methods on something with the right types?
|
|
||
| func GetExternalOutputFileName(inputFileName tspath.RootedFilePath, options *core.CompilerOptions, host OutputPathsHost) tspath.RootedFilePath { | ||
| outputPath := getOutputFileNameWithoutChangingExtension(inputFileName, options.OutDir, host) | ||
| rewritten, ok := core.RewriteExtension(outputPath.AsString(), host.ContentMapperExtensionRewrites(), host.CaseSensitivity().IsCaseInsensitive()) |
There was a problem hiding this comment.
Yeah, these conversions to and from strings don't feel super to me
| ImportName: "__rewriteRelativeImportExtension", | ||
| Scoped: false, | ||
| Text: `var __rewriteRelativeImportExtension = (this && this.__rewriteRelativeImportExtension) || function (path, preserveJsx) { | ||
| Text: `var __rewriteRelativeImportExtension = (this && this.__rewriteRelativeImportExtension) || function (path, preserveJsx, extraExtensions, ignoreCase) { |
There was a problem hiding this comment.
Have we ever extended an existing tslib entry with params before? Wondering if we need a new name; we of course need to now publish tslib with this, so I need to get that publish pipeline created, oops
|
Andrew Branch (@andrewbranch) Thanks for working on this. The output-extension support is a useful step forward.
I understand the preference for making the source format visible, but I don’t think that should rule out extensionless resolution when a project explicitly configures it and its build tool supports it. TypeScript already allows More fundamentally, The proposal in #64549 is deliberately limited to registered extensions and contexts that already support extensionless lookup. The resolver would use the registered extension list; the mapper would still process the file after resolution selects it. Node ESM’s explicit-extension requirements would remain intact. There are practical benefits beyond shorter imports. Extensionless paths let a component change source format without requiring changes throughout its import graph. They also preserve existing conventions in projects whose language tools already support this through Volar, avoiding import rewrites solely to migrate to TypeScript 7. The precedence and declaration-emission questions deserve explicit answers. However, they seem like reasons to define the feature’s constraints rather than require source extensions universally, especially when declaration emit is irrelevant to a I really hope you would consider an opt-in setting on a |
Fixes #64053
This makes TypeScript content mapping a better citizen in a system where another tool is going to emit content mapped files to disk. The current system basically assumes a bundler or running the source in place.
A content mapper's package.json can now specify
typescript.contentMapper.outputExtensions, a mapping from a content mapped file's original extension to the output extension that another tool will emit:{ "name": "ember-content-mapper", "typescript": { "contentMapper": { "exec": ["node", "server.js"], "outputExtensions": { ".gts": ".js" } } } }The same configuration is supported in
tsconfig.jsonas an override (the entire mapping is overridden; keys are not merged):{ "contentMappers": [ { "package": "ember-content-mapper", "extensions": [".gts"], "outputExtensions": { ".gts": ".js" } } ] }This causes:
The declaration file output for
Component.gtsto beComponent.d.tsinstead ofComponent.d.gts.ts, because there is expected to be aComponent.js(that someone else will create).rewriteRelativeImportExtensions: true. Why? If configuration indicates thatPage.gtsis going to be emitted asPage.js, then a plain TypeScript file in the same program that imports it:must be emitted as
which is exactly what
rewriteRelativeImportExtensionsdoes.The alternative would be to import from
"./Page.js"or even"./Page"depending on module resolution settings, i.e., an output-compatible path. That request is Content mappers: let registered extensions take part in extensionless module lookup #64549, and while this PR allows us space to consider that in the future, I strongly prefer imports that will trigger a content mapper be immediately recognizable by file extension.