This repository was archived by the owner on Feb 23, 2024. It is now read-only.
Refactor useForcedLayout to subscribe to store changes and to batch block insertions
#7879
Conversation
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
opr
added
status: needs review
type: refactor
rubikuserbot
requested review from
a team and
nielslange
and removed request for
a team
|
The release ZIP for this PR is accessible via: |
Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the |
TypeScript Errors ReportFiles with errors: 429 🎉 🎉 This PR does not introduce new TS errors. |
|
Size Change: +174 B (0%) Total Size: 975 kB
ℹ️ View Unchanged
|
nielslange
approved these changes
Dec 12, 2022
There was a problem hiding this comment.
I successfully tested this PR both using wp.org and wp.com.
I do have to say that on wp.com, I haven't been able to run npx @wordpress/create-block -t @woocommerce/extend-cart-checkout-block my-test-plugin. When trying to do that via SSH, I received the following error message:
bash: npx: command not foundApart from that, the PR was working as expected.
github-actions
bot
added
status: ready to merge
and removed
status: needs review
labels
|
I love the effort you put into testing the performance of this Thomas! |
senadir
approved these changes
Dec 14, 2022
No worries Niels, you can run that locally, cd into the new directory, run Thank you for testing @nielslange and @senadir |
4 tasks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Following on from the discussion in pca54o-4GY-p2#comment-4658 I implemented the suggestion of @jsnajdr. I altered
useForcedLayoutto subscribe to changes tocore/block-editorin the registry. I also batched the action dispatches to that store. Whereas we previously inserted blocks individually, we now insert multiple blocks at once if necessary.I added two new functions (the functionality already existed in the old
useForcedLayouthook, I just separated it):getMissingBlocksandfindBlockPosition. These functions find which registered blocks are missing from a forced layout, and find the index that these blocks should be inserted at, respectively.Fixes #7367
Testing
Automated Tests
Internal dev testing
@woocommerce/extend-cart-checkout-blocktemplate. (Go topluginsand exectue:npx @wordpress/create-block -t @woocommerce/extend-cart-checkout-block my-test-plugin).Enable tax rates and calculations)I want to receive updates about products and promotions.checkbox in the Contact Information block, ensure you see the VAT Number block rendered at the bottom of the Checkout block (this is intentional).User Facing Testing
WooCommerce Visibility
Performance Impact
I tested the performance of my version by logging the difference in
performance.now()between the start and end of thesubscribefunction (this is where most of the work gets done) and in the old version by logging the difference inperformance.now()in theuseLayoutEffect. I took an average of the values of every timestamp collected when inserting a new block and it averaged out around 10ms for both implementations.I also took the following measurements:
Changelog