Component Optimization: Pattern overrides hook

WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.

https://wordpress.org/gutenberg/

Other

10.3k stars 4.11k forks source link

Component Optimization: Pattern overrides hook #63146

Closed cbravobernal closed 2 weeks ago

cbravobernal commented 2 months ago

What problem does this address?

Right now, the code inside ControlsWithStoreSubscription function is executed on each block change (including writing a post, each keystroke or hovering a block) regardless the Override block is present or not (it appears only in patterns as far as I know) https://github.com/WordPress/gutenberg/blob/e39ae77a9ea59c3f41ee63f1f1f1de31209519d0/packages/editor/src/hooks/pattern-overrides.js#L95

https://github.com/WordPress/gutenberg/assets/37012961/84a885da-b8c8-4a6d-9bf1-24d2233c0777

How to test

Add a console log here: https://github.com/WordPress/gutenberg/blob/e39ae77a9ea59c3f41ee63f1f1f1de31209519d0/packages/editor/src/hooks/pattern-overrides.js#L95
Write a post with the browser console opened, check that the log appears tons of times.

ping @Mamaduka

Mamaduka commented 2 months ago

Generally, I think it's okay if a component rerenders like this unless it's costly to render and blocks the UI, which doesn't seem to be the case here.

As @ellatrix suggested in the Slack thread, we can optimize this using React.memo and only pass the required props. The props needed for the Control and its child components are clientId, name, metadata and setAttributes.

One issue with memoization is that it's easy to break. Let's say in the future someone updates the code and passes the whole attributes object as props. It will set us back to where we started with + a small cost of component memoization. But we'll have the same issue in the block-editor package, so maybe that's okay.

I created a draft PR just in case - #63189.

@kevin940726, @talldan, what do you think?

kevin940726 commented 1 month ago

Are the re-renders costly here? Are we fixing a performance bottleneck or doing premature optimizations? Could this be solved if we leverage the React Compiler in the future?

we can optimize this using React.memo and only pass the required props.

As pointed out, memoing also comes with a cost. We can audit it to see the trade-offs and whether it's worth it.

I'm not opposed to improving it in any way, but I think we should be careful of optimizing based on our assumptions without data support. I appreciate the draft PR explorations a lot! Thanks! ❤️

cbravobernal commented 2 weeks ago

The component has not been moved. Re-renders are not costly so we don't need any development here. Thanks @Mamaduka for your contribution!