Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Component Optimization: Pattern overrides hook #63146

Open
cbravobernal opened this issue Jul 4, 2024 · 2 comments
Open

Component Optimization: Pattern overrides hook #63146

cbravobernal opened this issue Jul 4, 2024 · 2 comments
Labels
[Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) [Type] Performance Related to performance efforts

Comments

@cbravobernal
Copy link
Contributor

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)

Screen.Recording.2024-07-04.at.14.31.36.mov

How to test

ping @Mamaduka

@cbravobernal cbravobernal added [Type] Performance Related to performance efforts [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) labels Jul 4, 2024
@Mamaduka
Copy link
Member

Mamaduka commented Jul 5, 2024

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
Copy link
Member

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! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) [Type] Performance Related to performance efforts
3 participants