Interactivity: Restore scope when yielded promise rejects #59708
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: +5 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
sirreal
force-pushed
the
fix/interactivity-async-generator-scope
branch
from
8b49642 to
5d08081
Compare
sirreal
commented
Mar 8, 2024
Comment on lines
+128
to
+133
| setNamespace( ns ); | ||
| setScope( scope ); | ||
| gen.throw( e ); | ||
| } finally { | ||
| resetScope(); | ||
| resetNamespace(); |
There was a problem hiding this comment.
@DAreRodz I tried 2 different approaches and they both fixed my test case. I wasn't sure why we have 2 try blocks, so my first version merge the try/finally and try/catch into one try/catch/finally. Then, since I wasn't sure what the motivation for 2 blocks were, I changed use 2 blocks. I'd like to get your insight here.
Both versions are on this branch in different commits and can be reviewed.
There was a problem hiding this comment.
I think this is the best approach: to have two separate try-catch blocks. The thing with async actions is that they can change the scope at any time―whenever they run―, thus leading to unexpected results when the scope is reset.
We decided here to set/reset the scope for synchronous code fragments, i.e., the generator code from one yield to the next one. So, from my understanding, we should reset the scope, then wait for the promise to be fulfilled (which will probably set their own scope), and if it fails, then set/reset the scope again.
sirreal
commented
Mar 8, 2024
packages/e2e-tests/plugins/interactive-blocks/generator-scope/render.php
Outdated
Show resolved
Hide resolved
sirreal
commented
Mar 8, 2024
| @@ -77,6 +77,7 @@ | |||
| <!-- Exclude PHPUnit tests from file and class name sniffs (for Core parity). --> | |||
| <rule ref="WordPress.Files.FileName.NotHyphenatedLowercase"> | |||
| <exclude-pattern>/phpunit/*</exclude-pattern> | |||
| <exclude-pattern>*\.asset\.php$</exclude-pattern> | |||
sirreal
requested review from
kevin940726,
Mamaduka,
ntwb,
nerrad,
ajitbohra,
luisherranz and
DAreRodz
as code owners
|
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. |
sirreal
added
[Type] Bug
|
@sirreal, nice job finding and proposing the fix for the bug. Unrelated to this PR. I'm a little bit afraid, that the current testing strategy doesn’t scale well. To run a single test, you need to write a new block in the first place. To run the test, you need to have a WordPress instance running, activate the plugin, add a post with the block, and then perform all the cleanup after the test. It all takes time and resources on CI, and isn't easy to run for contributors locally. What was the rationale for avoiding using the unit tests for the internal runtime implementation? |
|
I think that's a good point. I followed the existing testing pattern, but I'll look into adding some unit tests as well. Edit: I think it would be worthwhile to add some unit tests, but I'd like to land this PR without them. Implementing a unit test for this would require me to get much more familiar with the framework. I think it's worth it to get this fix into a release, ideally it can ship with WordPress 6.5. |
sirreal
added
the
Backport to WP 6.8 Beta/RC
That would be a big ask to define a unit testing strategy in a bug fix in one of the first contributions to the package 🙈 The code is running on millions of websites sites using interactive core blocks covered by existing e2e tests, so that can't be a blocker for this PR 😄 |
|
Since we're past RC1, only severe bug fixes are allowed into 6.5 at this point, so my question is how important it is for the current PR to land in 6.5. |
DAreRodz
approved these changes
Mar 11, 2024
@youknowriad, I think this fix would be similar to this bugfix. It's not like this bug breaks any block or site, but it breaks developers' expectations while using the Interactivity API. In that regard, I would say this bugfix "ensures that the contract provided by the Interactivity API is maintained". Asynchronous actions should keep the scope during their whole execution―even when exceptions arise―and the example shared by @sirreal in the description is a valid and expected use case. |
|
I agree with @DAreRodz, this is a fix that seems quite safe for a bug that I discovered very quickly when playing with the Interactivity API loosely based on some code samples in the Interactivity API dev note. I think it would be a shame to release 6.5 and the Interactivity API knowing that it has this bug. The bug is clear and the fix seems low risk. |
Co-authored-by: Anton Vlasenko <[email protected]>
|
I'm making this change and I'll add an entry to the package changelog, then I'll merge.
|
sirreal
force-pushed
the
fix/interactivity-async-generator-scope
branch
from
dc5fc36 to
242f138
Compare
getdave
pushed a commit
that referenced
this pull request
Mar 11, 2024
…59708) Ensure scope is properly restored when an exception is thrown in a generator callback. For example, the following block should have the correct context in the catch block: try { yield Promise.reject( '💥' ); } catch ( err ) { getContext().error = err; } --- Co-authored-by: sirreal <[email protected]> Co-authored-by: DAreRodz <[email protected]> Co-authored-by: gziolo <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: anton-vlasenko <[email protected]> # Conflicts: # packages/interactivity/CHANGELOG.md
|
I just manually cherry-picked this PR to the pick/wp-65-rc-2 branch to get it included in the next release |
getdave
added
Backported to WP Core
getdave
pushed a commit
that referenced
this pull request
Mar 12, 2024
…59708) Ensure scope is properly restored when an exception is thrown in a generator callback. For example, the following block should have the correct context in the catch block: try { yield Promise.reject( '💥' ); } catch ( err ) { getContext().error = err; } --- Co-authored-by: sirreal <[email protected]> Co-authored-by: DAreRodz <[email protected]> Co-authored-by: gziolo <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: anton-vlasenko <[email protected]> # Conflicts: # packages/interactivity/CHANGELOG.md
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Mar 27, 2024
…ordPress#59708) Ensure scope is properly restored when an exception is thrown in a generator callback. For example, the following block should have the correct context in the catch block: try { yield Promise.reject( '💥' ); } catch ( err ) { getContext().error = err; } --- Co-authored-by: sirreal <[email protected]> Co-authored-by: DAreRodz <[email protected]> Co-authored-by: gziolo <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: anton-vlasenko <[email protected]>
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?
Ensure that the interactivity scope is maintained when a promise rejects.
Why?
I expected to be able to do something like this in an interactivity callback:
But the
getContext()in the catch block was alwaysundefined.How?
Ensure that scope is properly set when handling rejection.
Testing Instructions
This includes an e2e test.