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

Improve waitFor() types #459

Merged
merged 1 commit into from Sep 8, 2023
Merged

Conversation

bendemboski
Copy link
Contributor

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.

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

Now that machty/ember-concurrency#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 Techn1x mentioned this pull request Aug 3, 2023
@Techn1x
Copy link

Techn1x commented Aug 7, 2023

I eagerly waitFor 😉 this to be merged too

@locks
Copy link

locks commented Aug 11, 2023

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

@locks locks self-assigned this Aug 11, 2023
@chrisvdp
Copy link

@locks any updates on deploying?

@kellyselden
Copy link
Member

@locks Anything we can do to help?

@locks
Copy link

locks commented Sep 8, 2023

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

scalvert commented Sep 8, 2023

I can merge and release if you need it.

@scalvert scalvert merged commit 790a4f6 into emberjs:master Sep 8, 2023
14 checks passed
@chrisvdp
Copy link

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

@scalvert
Copy link
Collaborator

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

Techn1x commented Sep 18, 2023

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

Techn1x commented Sep 25, 2023

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

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

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? 🥺

@NullVoxPopuli NullVoxPopuli added the enhancement New feature or request label Oct 25, 2023
@github-actions github-actions bot mentioned this pull request Mar 5, 2024
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

7 participants