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

Add glint types #69

Closed
wants to merge 21 commits into from
Closed

Add glint types #69

wants to merge 21 commits into from

Conversation

vlascik
Copy link
Contributor

@vlascik vlascik commented Feb 19, 2023

Types were taken from https://github.com/Gavant/glint-template-types/tree/main/types/ember-render-modifiers .

These depend on proper export of types for class modifiers from @ember/modifier, which seems be missing as of yet.

@RobbieTheWagner
Copy link
Member

@vlascik is there a way we can work around the @ember/modifier thing?

@vlascik
Copy link
Contributor Author

vlascik commented Apr 28, 2023

@vlascik is there a way we can work around the @ember/modifier thing?

As far as I can tell @ember/modifier now has the proper types & exports. No idea what the original problem was (did too many of these glint PRs at once), but it seems to be ok now, so this should probably be good to go.

@vlascik
Copy link
Contributor Author

vlascik commented Apr 28, 2023

I investigated a bit further, the issue was that the runtime of this addon imports from a package @ember/modifier, which doesn't really exist (might be some ember virtual package), so TS couldn't find any types for it. Real package is named ember-modifier, so I used that instead.

I also added the explicit dependency on ember-modifier and moved ember-auto-import to dependencies as well. The addon should now follow all the v1 addon recommendations from ember-cli-typescript and glint.

@esbanarango
Copy link

This is great!🙏

@RobbieTheWagner RobbieTheWagner added the enhancement New feature or request label Apr 28, 2023
@RobbieTheWagner
Copy link
Member

@vlascik this is awesome, thank you! It seems like CI isn't running though? I want to make sure all the tests pass before merging.

@vlascik
Copy link
Contributor Author

vlascik commented Apr 29, 2023

Well, seems that CI is not running on other PRs in this repo either.

This PR only adds a bunch of .d.ts files, and doesn't change anything related to CI (apart from updating a few packages).

FWIW ember test passes on my machine.

@RobbieTheWagner
Copy link
Member

@NullVoxPopuli @kategengler any ideas on why CI is not running?

@kategengler
Copy link
Member

@RobbieTheWagner Not exactly, but I have seen this on some repos where the scheduled workflow was suspended due to repo inactivity. It seems to affect PRs too. I've re-enabled that now. I think updating the PR may trigger CI

README.md Outdated Show resolved Hide resolved
@RobbieTheWagner
Copy link
Member

Thanks @kategengler seems to have worked!

@RobbieTheWagner
Copy link
Member

@vlascik I got CI all green now, so could you please rebase your PR?

@vlascik
Copy link
Contributor Author

vlascik commented May 2, 2023

@vlascik I got CI all green now, so could you please rebase your PR?

I tried, but messing with a new Git client seems that I ended up with merge instead. However there was only one conflict in yarn.lock, so maybe it will pass CI anyway. I can't start it though, only maintainers can.

If CI breaks I'll try to unravel it some other way.

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

Successfully merging this pull request may close these issues.

None yet

5 participants