Import of .gts emits broken declarations

[simonihmig]

As reported on Discord, there seems to be a bug when having an import with an explicit .gts extension and running glint --declaration.

Reproduction:

• go to https://stackblitz.com/edit/github-yl7xy1
• cd packages/accordion
• pnpm build
• in packages/accordion/declarations/components/accordion.d.ts you can see an import of item.gts, which does not resolve, as there is only an item.d.ts, which TS/Glint does not see as the declaration that matches this import apparently:

The emitted declarations can be fixed by patching manually, either

• by changing the import to be extension-less
• or by renaming item.d.ts to item.gts.d.ts

So I think glint --declaration should be doing either of these.

[NullVoxPopuli]

Demo:

• https://stackblitz.com/edit/stackblitz-starters-cj9ass?file=README.md
• https://stackblitz.com/edit/stackblitz-starters-g58nhp?file=README.md
  • this one tries moduleResolution Node16 and module Node16

[enspandi]

Not sure if useful for others, but I'm using this simple workaround for now:

Patch writeFile to search/replace .gts to .ts

```patch diff --git a/lib/cli/perform-check.js b/lib/cli/perform-check.js index 6bef94d92a5026f78073c8c35b6f8578df8e4173..632c5f3d2b98837b973c12dc28ff01b3346df8f1 100644 --- a/lib/cli/perform-check.js +++ b/lib/cli/perform-check.js @@ -47,6 +47,19 @@ function createCompilerHost(ts, options, transformManager) { host.fileExists = transformManager.fileExists; host.readFile = transformManager.readTransformedFile; host.readDirectory = transformManager.readDirectory; + + // Postprocess .d.ts files to temporarily patch '.gts' to '.ts' + // https://github.com/typed-ember/glint/issues/628 + const tsWriteFile = host.writeFile; + const matchGtsImport = /\.gts';/gm; + host.writeFile = (fileName, data, writeByteOrderMark, onError) => { + const isDts = fileName.endsWith('.d.ts'); + if (isDts && matchGtsImport.test(data)) { + data = data.replace(matchGtsImport, ".ts';"); + } + tsWriteFile(fileName, data, writeByteOrderMark, onError); + }; + return host; } function loadTsconfig(ts, transformManager, configPath, optionsToExtend) { diff --git a/lib/cli/perform-watch.js b/lib/cli/perform-watch.js index bdcd34a27c719dd2ab734cbb6ece51be27e33a43..9eef18740a7d8eecff33e51b8af3adde6a825284 100644 --- a/lib/cli/perform-watch.js +++ b/lib/cli/perform-watch.js @@ -9,6 +9,19 @@ export function performWatch(glintConfig, optionsToExtend) { let host = ts.createWatchCompilerHost(glintConfig.configPath, optionsToExtend, sysForCompilerHost(ts, transformManager), patchProgramBuilder(ts, transformManager, ts.createSemanticDiagnosticsBuilderProgram), (diagnostic) => console.error(formatDiagnostic(diagnostic))); // @ts-ignore: This hook was added in TS5, and is safely irrelevant in earlier versions. Once we drop support for 4.x, we can also remove this @ts-ignore comment. host.resolveModuleNameLiterals = transformManager.resolveModuleNameLiterals; + + // Postprocess .d.ts files to temporarily patch '.gts' to '.ts' + // https://github.com/typed-ember/glint/issues/628 + const tsWriteFile = host.writeFile; + const matchGtsImport = /\.gts';/gm; + host.writeFile = (fileName, data, writeByteOrderMark, onError) => { + const isDts = fileName.endsWith('.d.ts'); + if (isDts && matchGtsImport.test(data)) { + data = data.replace(matchGtsImport, ".ts';"); + } + tsWriteFile(fileName, data, writeByteOrderMark, onError); + }; + ts.createWatchProgram(host); } ```

[NullVoxPopuli]

Generally, why my idea of "just tossing .gts in as 'part of the file name'" won't work:

• https://discord.com/channels/480462759797063690/568935504288940056/1171838869914779659

[NullVoxPopuli]

Made a tool to help out: https://github.com/NullVoxPopuli/fix-bad-declaration-output

[bartocc]

One simple workaround I am using is to do 2 separate imports.

• One with the .gts exension for the value I want to use, for example, a component
• One without the .gts exension for the type

The result is something like this 👇 and the generated .d.ts does not reference a .gts file anymore

import { type TOC } from '@ember/component/template-only';
import MyButton from "./button.gts"
import type MyButton628Hack from "./button"

interfase Signature {
  Blocks: {default: [typeof MyButton628Hack]}
}

const FooComponent: TOC<Signature> = <template>
  {{yield MyButton}}
</template>

export default FooComponent;
}

[chriskrycho]

The solution here appears to be just as simple as making sure that when doing --declaration, the emitted file has the full specifier name, right? .gts.d.ts should do the trick just fine. That means you don’t have to muck with the import specifier at all; TypeScript will resolve it correctly out of the box—and then bundlers can operate exactly as they want to with fully-resolvable module specifiers.

[NullVoxPopuli]

correct, however, @dfreeman mentioned that if someone imports a file with the extension in one place, but then without the extension in another place, then you can't have the import resolve correctly in both places.

ofc, the solution is to not do that, or generate two files.

[dfreeman]

I think making this a flag that determines what path we use when emitting declarations for non-TS extensions, as per the discussion we had last fall in Discord and on in this comment thread, is a very straightforward solution.

[chriskrycho]

if someone imports a file with the extension in one place, but then without the extension in another place, then you can't have the import resolve correctly in both places.

This is… just how TS and ESM work. Along with every other custom file extension (.svelte, .vue, heck even .css with CSS type code-gen). So… yeah. :joy:

[NullVoxPopuli]

exactly, which is why I don't think we need a flag, which is a disagreement, which is why I'm working around the problem right now, because I feel stuck with Glint :_\

[dfreeman]

I hope “don’t break existing code” isn’t a controversial take, and I’m unclear on how that’s a source of disagreement.

[NullVoxPopuli]

It's just that no one using gts in their imports (everyone with v2 addons using @embroider/addon-dev@4+) has working code / declarations (without additional non-glint post-processing) -- so there is nothing to protect via flag 🤷

[dfreeman]

I know you want that to be true, but we've been through this conversation already: https://discord.com/channels/480462759797063690/491905849405472769/1166823826382934117

And the existence of one example in public code suggests there may well be other non-public ones as well. I don't see how leaving Glint broken is preferable to introducing a flag.

[NullVoxPopuli]

I don't see how leaving Glint broken is preferable to introducing a flag

Not really a choice, i did attempt to fix the problem, but i couldn't figure it out. I'm not smart enough for Glint :/ (or at least, i don't have the time to figure it all out. feels like learning an asp.net level framework in a language i don't yet know - a bit much for spare time)

If someone (not me) submitted a PR that provided a fix, that'd be great. I wouldn't mind if they gated behavior behind a flag.

[NullVoxPopuli]

Here is how I'm fixing this: https://github.com/NullVoxPopuli/fix-bad-declaration-output

easiest, quickest, we can all move forward 🎉

example usage: https://github.com/universal-ember/ember-primitives/blob/main/ember-primitives/rollup.config.mjs#L34-L52

[simonihmig]

Would be better to fix the root cause of course, but I don't need to tell you that, and I also have no time and skills to get this fixed upstream :/

But for end users, the plan is to land this as part of https://github.com/embroider-build/embroider/pull/1798, right?

[NullVoxPopuli]

Eventually, that'd be ideal, yea. I couldn't figure out how to get tsc to be ok with await import though :( (tsc kept changing it to require)

I also don't have time to figure out the dual-build goals for embroider atm, which would help the esm case, but not cjs.

Hopefully there is a flag or setting that allows author-as-ESM-but-actually-its-cjs-when-compiled to retain await import.

[machty]

FYI as a byproduct of volar-izing glint, the declaration files emitted for basename.gts are now basename.gts.d.ts. This is in line with how Vue and others do it, and should serve as a solution to this issue, though it will likely be a breaking change for certain apps. We're moving forward with this as a solution, and depending on a number of factors, we may decide to introduce a config or mitigating solution at some point in the future if this change is too much.

I'll let someone else decide if this is worth closing, but at this point the change to .gts.d.ts has already been merged.