Improve handling of exceptions thrown during act callback#2433
Merged
robertknight merged 5 commits intomasterfrom Mar 31, 2020
Merged
Improve handling of exceptions thrown during act callback#2433robertknight merged 5 commits intomasterfrom
act callback#2433robertknight merged 5 commits intomasterfrom
Conversation
If act's callback throws an exception, sync or async, make sure that any pending flushes of the render or effect queues are executed. If this is not done, Preact will be left in a state where it has a non-empty update/effect queue and so future deferred updates/effects will not trigger another flush since one is already pending. However the already-pending update will never get flushed, since `act` had overridden the default flushing mechanism. As a result, deferred updates and effects scheduled later don't get run. Fixes #2432
Make sure that future state updates and effects continue to run if an exception occurs during a call to `act`. This required resolving an issue with hooks that an exception inside `useEffect` left the effect callback in the effect queue, so it would run again the next time effects were flushed.
|
Size Change: +140 B (0%) Total Size: 38.6 kB
ℹ️ View Unchanged
|
The Preact tests generally don't use `context` blocks and the ESLint rules don't know to treat a `context` block like `describe` (see jest-community/eslint-plugin-jest#83)
Duplicate the logic to reset effects in the `catch` branch per recommendation at #2433 (review) to save a few bytes, rather than using a variable.
JoviDeCroock
approved these changes
Mar 31, 2020
Member
JoviDeCroock
left a comment
There was a problem hiding this comment.
Looks good to me, nice work!
Member
Author
|
Thanks for the quick review! |
Member
|
Great work! That's a very nice improvement!! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes #2432 by ensuring that effect and pending render queues are flushed if a callback passed to
actthrows an exception. PreviouslyuseEffecteffects and state updates would stop working after a call toactaborted with an exception. The result was that a single failure in a test suite could trigger a cascade of failures in unrelated tests, which was pretty confusing.In the process, I discovered an issue that if an effect threw an exception, the effect would remain in the component's effect queue and would end up running again the next time that component's effects were flushed.