Passing `...this.props` to child components causes unnecessary re-renders?

[iandunn]

We do stuff like this a lot:

<SessionsBlockControls
    icon={ ICON }
    { ...this.props }
    { ...this.state }
/>

I wonder if that causes SessionsBlockControls to re-render unnecessarily whenever something in props or state changes? It looks like the Core blocks just pass in the exact values that are needed by the child, rather than everything.

wp.compose.pure might avoid that, but I don't see any Core blocks using it, which makes me wonder if it's not intended to be used as often as I would think from reading about pure components. It seems like maybe just not passing in extra stuff would avoid re-renders without the need for extra memoization?

[coreymckrill]

Yeah, Andrew recommended against it, and I moved away from it in several instances, but not all. It's probably worth auditing.

[coreymckrill]

I wonder if using context might be a way to resolve this issue.

[iandunn]

I haven't played around with Context much, but my first impression is that it has a lot of the awkwardness of HOCs, rather than being the elegant solution I hoped it'd be. It seems changing these so that we're only passing down the props we actually use would fix it, right? That seems more straight-forward and maintainable.

Having said that, prop drilling is awkward too, so I'd be open to exploring Context more.

I wonder if the way things are currently structured might be part of the problem as well. Maybe we could simplify the component hierarchy, or maybe some state is higher up than it should be?

@ryelle , what do you think?

[ryelle]

I wonder if the way things are currently structured might be part of the problem as well.

There's a lot of abstraction here (which is useful, the reusable components are nice), but I do think there's too much ...this.props-ing in general and it's hard to trace where a prop came from, and what props a component actually needs.

We should be explicit about which props are needed for each component - I tend to think Context would only hide that more, and I'm not sure what else we'd gain there.

[ryelle]

I wonder if we could use context as a "read only"-ish store for blockData, where we set blockData (and maybe ICON/LABEL) in the Edit component, and then each shared component can read the data from there, rather than relying on props.blockData

[coreymckrill]

setAttributes and entities are basically "read only" too, aren't they? attributes definitely aren't, but maybe that would be the only thing that we'd need to pass down through the hierarchy.

[ryelle]

It looks like withSelect triggers the multiple renders– I tested using a basic block that added a counter on render. In this example, it only ran once:

registerBlockType( 'wordcamp/test', {
    title: 'Test',
    category: 'wordcamp',
    edit: () => {
        console.count( 'Test Block Render' );
        return <div>Test block</div>;
    },
    save: () => null,
} );

But if I wrap it in the withSelect, copied from organizers, it jumps to 9 times.

import { withSelect } from '@wordpress/data';
import { WC_BLOCKS_STORE } from './data';

registerBlockType( 'wordcamp/test', {
    title: 'Test',
    category: 'wordcamp',
    edit: withSelect( ( select ) => {
        const { getEntities } = select( WC_BLOCKS_STORE );

        const entities = {
            wcb_organizer: getEntities( 'postType', 'wcb_organizer', { _embed: true } ),
            wcb_organizer_team: getEntities( 'taxonomy', 'wcb_organizer_team' ),
        };

        return {
            entities,
        };
    } )( () => {
        console.count( 'Test Block Render' );
        return <div>Test block</div>;
    } ),
    save: () => null,
} );

If I comment out one of the getEntities calls, it drops to 6, and if I swap our call for a core data call, I still see 6 renders. I'm not going to dive deeper here right now, but I wanted to leave these notes somewhere 🙂

[iandunn]

It seems like heartbeat requests also trigger re-renders every 30 seconds or whatever, even though no props have changed.