react-refetch icon indicating copy to clipboard operation
react-refetch copied to clipboard

Fix a bug regarding the injected props

Open gwn opened this issue 6 years ago • 3 comments

While injecting props to the wrapped component, prioritize props that come from the HOC state over props that are passed from the parent.

Not sure if this is a bug, or if my "fix" breaks something else, but here is the reason I needed this:

I'm doing server-side rendering; and in addition to decorating my components with React Refetch's connect for my client-side fetching needs; I also developed another HOC with a nearly same API called withInitialRequest that is able to do fetching on the server side as well:

compose(
    withInitialRequest(() => ({
        brandsFetch: '/admin/brands',
    })),

    connect(() => ({
        ...
    })),
)(SomeComponent)

What I need is the ability to override the content of that brandsFetch created by withInitialRequest using a lazy refresh method defined in connect:

compose(
    withInitialRequest(() => ({
        brandsFetch: '/admin/brands',
    })),

    connect(() => ({
        refreshBrands: page => ({
            brandsFetch: '/admin/brands?page=' + page,
        }),
    })),
)(SomeComponent)

But this doesn't work, because the HOC returned from connect favors parent props over the props it generates from its state in case of a name collision while rendering the child component. So SomeComponent above can never get a "refreshed" brandsFetch, as it is stuck with the brandsFetch coming from the upper HOC.

The obvious workaround is this:

compose(
    withInitialRequest(() => ({
        brandsInitialFetch: '/admin/brands',
    })),

    connect(({brandsInitialFetch}) => ({
        brandsFetch: {value: brandsInitialFetch.value},

        refreshBrands: page => ({
            brandsFetch: '/admin/brands?page=' + page,
        }),
    })),
)(SomeComponent)

But this is boilerplate I'd rather not duplicate everywhere.

I think it generally makes sense that the props generated by the HOC itself should override the props coming from the parent and not the other way around. I feel that the inner wrapper should have priority in case of name collisions.

Looking forward to your thoughts.

Great library, btw. : )

gwn avatar Feb 08 '19 00:02 gwn

Thanks for this. While it does make sense, I'm afraid of this potentially breaking existing apps. What do you think about having this as a configured option?

ryanbrainard avatar Mar 11 '19 05:03 ryanbrainard

Thank you for taking the time! I agree that it may break some apps. Having a configured option makes sense to me.

The challenge is, what can we name that option? :)

overrideParentProps, preferRefetchProps, preferRefetchPropsOnConflict; are some ugly suggestions.

gwn avatar Mar 11 '19 13:03 gwn

This is something that I definitely want and I expected it to behave like this from default, similar to how recompose' hocs are working, e.g. withProps and withHandlers.

I'm for overrideParentProps and this to be released soon :)

ipanasenko avatar May 22 '19 14:05 ipanasenko