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

Fix support for gts extensions remaining in emitted declarations #648

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Nov 7, 2023

Resolves: #628


Given imports of:

// index.ts
import { Greeting } from './greeting.gts';
import { greeter } from './greeter.ts';

emitted declarations created via glint --declaration contain:

// index.d.ts
import { Greeting } from './greeting.gts';
import { greeter } from './greeter.ts';

This is incorrect, as in order to make greeting.gts and greeter.ts valid imports in declarations, the following declaration files would need to exist:

- ./index.d.ts <- the entrypoint (pictured above)
- ./greeting.gts.d.ts
- ./greeter.d.ts <- tsc has built in logic for this

A goal is not just have Glint support gjs/gts imports, but have the emitted output of glint --declarations be compatible with tsc.

So, to do that, for .gjs and .gts imports, we'll need to include .gjs and .gts in the name of the emitted assets.

the old approach

So one way to deal with this is to, when transforming the files, chop off the extensions entirely so that the generated index.d.ts would contain:

// index.d.ts
import { Greeting } from './greeting';
import { greeter } from './greeter';

which means that, the currently emitted .d.ts-only files would resolve...
if the resolving code is changed to support iterating over all possible file extensions -- which could lead to slower declaration building.

background

using the learnings from work on the rollup plugins, it was much easier to work with all the different files with explicit extensions in the imports.

Resolving got way easier, because the extensions were just right there in the import, and correct to what matched on disk.

Unlike how TS has totally make extensions imports a nightmare for some projects, extension usage on @embroider/addon-dev makes sense (specifying the author format, and it's compiled away in the resulting js).


I think it'd be easier and less code to emit basename(<import specifier>).d.ts for every import, than to

  • remove the extension from all imports
  • add resolving code everywhere to re-find the files

(which would def be a breaking change without a flag to opt in to this behavior)


  • failing test
  • fix
    • add tsconfig.json flag

Support for resolving gts-extensioned files was added in: #621
This PR finishes that work by making sure emitted declarations can resolve other declarations.

const rewritten = rewriteModule(ts, { script: source }, glimmerxEnvironment)!;
let rewritten;

beforeEach(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without these changes, debugging was super troll time

'barrel.ts': stripIndent`
export { default as Greeting } from './Greeting.gts';
export { Greeting as Greeting2 } from './re-export.gts';
export { two } from './vanilla.ts';
Copy link
Contributor Author

@NullVoxPopuli NullVoxPopuli Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the declarations for this are, at present, emitted as ./vanilla.ts, which is wrong.

To fix, either needs to happen:

  • remove the .ts extension so vanilla.d.ts matches with the emitted corresponding vanilla.js
  • or emit a goofy vanilla.ts.d.ts file, along the same lines as what I suggested originally with emitting Greeting.gts.d.ts

This PR is currently going the way of removing all extensions.

@NullVoxPopuli
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Import of .gts emits broken declarations
2 participants