Block editor: create automatic change higher order reducer #48312
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
ellatrix
commented
Feb 22, 2023
| selectionStart === state.selectionStart && | ||
| selectionEnd === state.selectionEnd | ||
| ) { | ||
| return state; |
There was a problem hiding this comment.
I also noticed that we were continuously creating a new object here. I'm curious if this improves performance.
There was a problem hiding this comment.
It would if there are selectors using the parent key as memoization key.
|
Size Change: +464 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Feb 22, 2023
|
|
||
| if ( | ||
| nextState.blocks === state.blocks && | ||
| nextState.selection === state.selection |
There was a problem hiding this comment.
So the question is: Is there potentially an action that updates either selection or blocks but shouldn't update "automaticChangeStatus"?
There was a problem hiding this comment.
So far every time we had this issue it's because of some other non content state
youknowriad
reviewed
Feb 22, 2023
|
Flaky tests detected in 43b0f74. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4244683455
|
youknowriad
approved these changes
Feb 22, 2023
kevin940726
reviewed
Feb 23, 2023
Comment on lines
+185
to
+187
| // Wait until the automatic change is marked as "final", which is done | ||
| // with an idle callback, see __unstableMarkAutomaticChange. | ||
| await page.evaluate( () => new Promise( window.requestIdleCallback ) ); |
There was a problem hiding this comment.
This is not something we want to put in e2e tests, since it's implementation details and not what end users could interact with. It can potentially also be an issue for end users if they press the ArrowUp key fast enough, right? Ideally, we should fix it in the code so that it can be deterministic. If that's not possible at the time then we should think along the line with retrying and waiting for the results, something like this:
await expect( async () => {
await page.keyboard.press( 'ArrowUp' );
expect(
await page.evaluate( () =>
window.wp.data.select( 'core/block-editor' ).getSelectedBlock()
)
).toMatchObject( { name: 'core/list' } );
} ).toPass();There was a problem hiding this comment.
It's not really an issue for a user: if selection changes too quickly, it's just that it will still be possible to undo the automatic change. We have a few of these lines throughout the e2e test where we have to wait for selection. We also have undo levels for rich text after 1 second of inactivity. How would we write an e2e test for something like that? Let's look at these all together separately.
There was a problem hiding this comment.
(I'd rather improve test stability for trunk right now.)
There was a problem hiding this comment.
Please follow up with a PR to fix this then.
We have a few of these lines throughout the e2e test where we have to wait for selection.
And this is the anti-pattern that I'm recommending against.
We also have undo levels for rich text after 1 second of inactivity. How would we write an e2e test for something like that?
Some of these can be mocked to reduce the time we have to wait. If that's hard-coded then we can always fallback to the pattern I mentioned above: wait for feedback that's accessible to the end users.
There was a problem hiding this comment.
I think we should avoid mocking entirely in e2e tests if possible otherwise it's not e2e testing anymore. If the user has to wait, let's make the test wait as well.
There was a problem hiding this comment.
Agreed. But sometimes it's inevitable, like caching and nonce tests, or just isolating for more stable tests. Waiting is okay, we just need to wait for user-visible behaviors instead.
There was a problem hiding this comment.
Hi @ellatrix, friendly reminder that this exists ❤️ . Feel free to request my reviews in the PR and we can discuss alternatives there more!
There was a problem hiding this comment.
I'm not really sure what you're asking here. I added the best solution to my knowledge in this PR. What does the code above do? It just keeps pressing up until the test passes? To me that doesn't feel like an e2e test?
There was a problem hiding this comment.
What to do is up to you, just don't rely on implementation details like requestIdleCallback. This could potentially be very flaky when React changes how it schedule async work for instance. Why doesn't it feel like e2e to you? Retrying and waiting on user-visible interactions sounds exactly like how end users would use the app to me. If retrying is not suitable here, try waiting on user-visible actions (like caret position) before pressing the button.
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?
Creates an automatic change higher order reducer so we can reset
automaticChangeStatusif blocks or selection changes.Why?
The current implementation is very fragile: if you add an action to the store, it will automatically reset the
automaticChangeStatuswhen dispatched.How?
Higher order reducer
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast