Skip to content

Remove runloop binding for perform helper #414

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 2 commits into from
Mar 17, 2021

Conversation

maxfierke
Copy link
Collaborator

Wrapping perform in a full runloop means that a synchronous task throw will be
reported within that same invocation, before another helper is able to wrap and
instrument the task instance. Since it's already run, it's too late to do things
like attaching catch handlers. We do not need this runloop binding anymore, since
there's no auto-run assertion with the versions supported by ember-concurrency
2.x.

Fixes #409

alexlafroscia and others added 2 commits March 15, 2021 12:55

Verified

This commit was signed with the committer’s verified signature.
romainmenke Romain Menke
Wrapping `perform` in a full runloop means that a synchronous task throw will be
reported within that same invocation, before another helper is able to wrap and
instrument the task instance. Since it's already run, it's too late to do things
like attaching catch handles. We do not need this runloop binding anymore, since
there's no auto-run assertion with the versions supported by ember-concurrency
2.x.

Co-authored-by: Max Fierke <max@maxfierke.com>
Co-authored-by: Alex LaFroscia <alex@lafroscia.com>
@maxfierke
Copy link
Collaborator Author

@alexlafroscia mind giving this a spin against your app to see if it resolves the problem "in the wild", so to speak?

@alexlafroscia
Copy link
Contributor

alexlafroscia commented Mar 16, 2021

Can do! I'll check this branch out tomorrow and see what happens

Copy link
Contributor

@alexlafroscia alexlafroscia left a comment

Choose a reason for hiding this comment

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

This solved the problem I was having in my app 🎉

Comment on lines +9 to +10
cancelableSymbol,
} from "../yieldables";
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed this too when working in the repo -- it might be nice to add Prettier to the linter config. It's harder working in repos where it isn't present than to work in ones where it is!

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy to put that PR together, if you're interested

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Definitely! Big fan of prettier, I just hate setting it up myself 😆

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.

Async Error Handler Running Synchronously
2 participants