emberjs / ember-test-waiters

An Ember addon to allow @ember/test-helpers to manage asynchronous operations
MIT License
29 stars 15 forks source link

Improve waitFor() types #459

Closed bendemboski closed 1 year ago

bendemboski commented 1 year ago

Use type arguments & inference to ensure that waitFor() in function form returns the same type that it was passed. This prevents it from getting in the way of passing functions as as typed arguments. For example, ember-concurrency's task() function expects an async arrow function as its argument, so task(async () => null) works, but without this change, task(waitFor(() => null)) does not because waitFor(() => null) is just Function.

bendemboski commented 1 year ago

Now that https://github.com/machty/ember-concurrency/pull/536 has been merged and released, it's be really nice to get this merged and released so people can start using the new syntax in Typescript. What needs to happen to move this forward?

Techn1x commented 1 year ago

I eagerly waitFor 😉 this to be merged too

locks commented 1 year ago

I'm not merging this for now until I have a guarantee it is getting deployed. Looking into it!

chrisvdp commented 1 year ago

@locks any updates on deploying?

kellyselden commented 1 year ago

@locks Anything we can do to help?

locks commented 1 year ago

I'ved ping you on Discord. In short I need help geting in touch with @scalvert (only contributor listed) so more people are added to the npm org.

scalvert commented 1 year ago

I can merge and release if you need it.

chrisvdp commented 11 months ago

@scalvert I think this still needs to be released. I don't see it here https://www.npmjs.com/package/@ember/test-waiters

scalvert commented 11 months ago

I'm waiting on a few other PRs before a release, since some of those require a major bump -> https://github.com/emberjs/ember-test-waiters/pulls/NullVoxPopuli.

Type changes constitute a major breaking change, so it makes sense to do them all at once.

Techn1x commented 11 months ago

How about releasing this as a major now and then another major when those PRs are ready and merged?

That way folks get access to this long awaited update (pun intended), especially if the other changes require more involved breaking changes that they may not be able to consume straight away.

Techn1x commented 11 months ago

I'm waiting on a few other PRs before a release, since some of those require a major bump

Hey @NullVoxPopuli do you want to weigh in here? Should we be waiting for your other PRs before cutting a version or just go for it?

NullVoxPopuli commented 11 months ago

Sowe of them aren't blocked on merging or anything, tbh, i thought no one was looking at the repo atm (apologies! I see i missed a message from two weeks ago!), so i hadn't prioritized finishing the red prs. 😅

If we want to start merging things, i can prioritize getting the v2 conversion accross the line, because it's part of a top-of-list effort for me for Polaris - getting all blueprint addons to v2.

bendemboski commented 11 months ago

If I'm reading things right, I think I want to argue that those other PRs are great/much needed infra updates, but aren't changes that are blocking consumers of this addon from doing anything, while the waitFor() fix is blocking consumers of this addon from using the async arrow function ember-concurrency task syntax, so it seems wrong to delay the stuff that blocking people for the stuff that isn't.

Also, while the waitFor() change is technically blocking, I'm pretty confident it won't actually require any upgrade work -- the types were just broken and unusable in wrapper form before, and now they work. So it's not like there's a user-facing benefit in batching up the breaking changes so they can do all the needed changes at once.

So can we please release this bugfix that's been fixed for almost two months? 🥺