Skip to content

Add glint support #107

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

Merged
merged 19 commits into from
Aug 17, 2023
Merged

Add glint support #107

merged 19 commits into from
Aug 17, 2023

Conversation

gossi
Copy link
Contributor

@gossi gossi commented Jul 25, 2023

This PR:

... turns out, the CI wasn't running for a while and this at least has some problems locally - it might be, the current main branch is broken, too.

@gossi gossi changed the title Adding glint support Add glint support Jul 25, 2023
@gossi
Copy link
Contributor Author

gossi commented Jul 25, 2023

Oh, I should've looked around at first. I found there is also #107 which adds glint support. I took note and add the registry (as I forgot in the first place). Now, we have registry + export from index :)

@NullVoxPopuli NullVoxPopuli added the enhancement New feature or request label Jul 25, 2023
@NullVoxPopuli
Copy link
Collaborator

Can you make this pr only about the glint types?

Upgrading embroider requires that we have a correct dep graph, and yarn doesn't provide that (nor is it capable)

Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

This is looking amazing!!!!! thank you!!!!

Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

looks good to me, thanks for working on this!

Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

oh but wait -- we need to run a typescript matrix in Ci, to assure we have TS compatibility.

I usually do TS 4.8, 4.9, 5.0, and 5.1 when running glint on the test-app

here is an example: https://github.com/NullVoxPopuli/ember-resources/blob/main/.github/workflows/ci.yml#L50-L76

Comment on lines 4 to 12
interface ElementReceiverSignature<T extends string> {
Element: ElementFromTagName<T>;
Args: {
tag: ElementSignature<T>['Return'];
};
Blocks: {
default: [];
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is my test type, but they appear not to work perfectly. Here is the glint error message:

app/components/element-receiver.hbs:2:3 - error TS2345: Argument of type 'NonNullable<ElementFromTagName<T>> extends never ? unknown : ElementFromTagName<T>' is not assignable to parameter of type 'Element'.
  Type 'unknown' is not assignable to type 'Element'.

2   <Tag id="content" ...attributes>{{yield}}</Tag>
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

app/components/element-receiver.hbs:2:21 - error TS2345: Argument of type 'NonNullable<ElementFromTagName<T>> extends never ? unknown : ElementFromTagName<T>' is not assignable to parameter of type 'Element'.
  Type 'unknown' is not assignable to type 'Element'.

2   <Tag id="content" ...attributes>{{yield}}</Tag>


> **Note:** Glint itself is still under active development, and as such breaking changes might occur.
> Therefore, Glint support by this addon is also considered experimental, and not covered by our SemVer contract!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Types docs start here. This is giving me problems. See below: https://github.com/tildeio/ember-element-helper/pull/107/files#r1286107261

@gossi
Copy link
Contributor Author

gossi commented Aug 8, 2023

Typechecks for TS 4.8+ with Glint. Had to use {{!@glint-ignore}} due to a found, which is documented at typed-ember/glint#610.

Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

Gonna try to get a review from the TS folks as well -- since the impact of this library is so wide

@gossi
Copy link
Contributor Author

gossi commented Aug 17, 2023

The problem is more likely to sit in upstream @glint/template. I tested the suggested change (as per draft PR: typed-ember/glint#613) locally (see test protocol in linked PR).

@NullVoxPopuli NullVoxPopuli merged commit 608873a into tildeio:main Aug 17, 2023
@NullVoxPopuli NullVoxPopuli mentioned this pull request Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants