Fix captured errors in withScope generators
#5
Conversation
This file contains 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
|
Size Change: +28 B (0%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
luisherranz
approved these changes
Mar 13, 2025
There was a problem hiding this comment.
Looks good to me 🙂
There are several redundant tests that I think we could eliminate, and apart from that, I don't quite understand why, if we go back to the previous implementation, the scope is not resetting correctly in the generators (according to the tests).
|
Thanks for the review, @luisherranz. 😊
I see. My idea was to test two things: the final returned value and the scope being correctly set for each step. That's why I created two separate tests. However, I just realized I'm testing both things in the scope tests. 😅 I'll remove those where only the result is tested and rename the others.
That's actually another bug! Look at this gutenberg/packages/interactivity/src/utils.ts Lines 156 to 165 in 116aad9 You can see the scope and namespace are set when an exception is caught, but they are always reset inside |
|
Regarding your question in the video about why we haven't noticed this before. 🤔 My guess is that, as the async actions and callbacks set their scope when they're called or resumed, it's not really necessary to preserve the previous scope (most of the time). Even if the previous one had disappeared, these functions are able to recover their scope and namespace. That's at least for functions wrapped with |
|
I'm not sure what's happening with the failing e2e tests. Although they seem entirely unrelated, they fail consistently. I'll try syncing this repo with the original one and updating the branch. |
|
All green. ✅ Merging the PR. |
|
Cool! Thanks for the explanations, David. Great job! 🙂 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What?
This PR fixes an issue in the
withScopefunction when it creates wrappers around async actions and callbacks defined in a store. In particular, the wrapper passes the wrong value to the generator in ayieldoperator that appears after an error is caught.Also, other fixes related to errors have been addressed:
nextorthrowfunctions are called and throw. This is the equivalent of an uncaptured error in the generator's code.return) is a rejected promise, similar to async functions.Why?
Async functions in the store should allow error handling properly.
Testing Instructions
E2E and unit tests have been added.