Skip to content
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

Import of .gts emits broken declarations #628

Closed
simonihmig opened this issue Oct 6, 2023 · 18 comments · May be fixed by #659
Closed

Import of .gts emits broken declarations #628

simonihmig opened this issue Oct 6, 2023 · 18 comments · May be fixed by #659

Comments

@simonihmig
Copy link
Contributor

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:
    image

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
Copy link
Contributor

NullVoxPopuli commented Nov 8, 2023

@enspandi
Copy link

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

Patch writeFile to search/replace .gts to .ts
  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
Copy link
Contributor

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

@NullVoxPopuli
Copy link
Contributor

@bartocc
Copy link

bartocc commented Jan 11, 2024

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member

chriskrycho commented Feb 15, 2024

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. 😂

@NullVoxPopuli
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

NullVoxPopuli commented Feb 16, 2024

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
Copy link
Member

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
Copy link
Contributor

NullVoxPopuli commented Feb 16, 2024

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
Copy link
Contributor

@simonihmig
Copy link
Contributor Author

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 embroider-build/embroider#1798, right?

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Jun 7, 2024

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
Copy link
Contributor

machty commented Jul 3, 2024

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.

@machty machty closed this as completed Sep 25, 2024
simonihmig added a commit to embroider-build/embroider that referenced this issue Dec 9, 2024
This invokes glint as a rollup plugin, and contains a workaround for broken gts imports, see typed-ember/glint#628
simonihmig added a commit to embroider-build/embroider that referenced this issue Dec 9, 2024
This invokes glint as a rollup plugin, and contains a workaround for broken gts imports, see typed-ember/glint#628
simonihmig added a commit to embroider-build/embroider that referenced this issue Dec 10, 2024
This invokes glint as a rollup plugin, and contains a workaround for broken gts imports, see typed-ember/glint#628
simonihmig added a commit to embroider-build/embroider that referenced this issue Dec 11, 2024
This invokes glint as a rollup plugin, and contains a workaround for broken gts imports, see typed-ember/glint#628
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants