Relay: [Modern] RefetchContainer disposes child RefetchContainer refetch

[jardakotesovec]

I had issue with RefetchContainer that it sometime (randomly) did not update after fetch.

I have found out that I have also RefetchContainer higher in component hierarchy and if these two RefetchContainers starts refetching roughly at the same time, there is good chance that RefetchContainer which is higher in the hierarchy will finish sooner and dispose the refetch of the lower RefetchContainer.

Reason is that parent RefetchContainer updates this.context.relay.variables, which hits the condition in children RefetchContainer, which calls this._release which dispose any pending refetches.

Not sure how this should be addressed. I assume that propagating the variables has also its own use case. Any insights very appreciated. But having multiple RefetchContainer in parent children relationship also seems are reasonable/common use case.

[sibelius]

related to https://github.com/facebook/relay/issues/2071

[leebyron]

Interesting!

I could see other variants of this behavior being problematic as well. For example, if the child RefetchContainer did not dispose it's refetch upon being rerendered, then it's refetch could be returned with incorrect results and the app could end up in an inconsistent state.

Also - in your example if the child refetch completes before the parent, then its results would still only be briefly seen, because when the parent refetch completes it would result in re-rendering the child.

What outcome are you looking to see? I'm labeling this as a bug while we investigate - but really this is just a limitation of the current known behavior of RefetchContainer, but that's not to say we couldn't make it better if we can understand this case

[yuzhi]

@leebyron I commented on the other issue around the variables getting reset. Essentially when a parent refetches, it doesn't really know which of it's children has updated variables, so the parent can only refetch with the initial variables for the children. Because of this, we reset the variables for the children to the variables that were used for the refetch. Relay can't guarantee that all the data would still be available for each of the children with the variables before the reset.
I'm not sure what the right outcome would be with the variables. Given the current behavior, it doesn't make sense to not dispose the child's request.

[sibelius]

In my case, I was using RefetchContainer to do pagination, using the renderVariable props.

I think in this case, the refetch should not override the already fetched data.

My problem was that I ask for 10 more edges in the connection, but the refetch resets any other data of children fragment already refetched

[jardakotesovec]

Thanks all for feedback. Really helpful.

I agree that prevent disposing would not work for all use cases.

Will try to share my use case as clearly as possible. Overview is that my parent RefetchContainer refetch just some field (which has argument), not everything. Therefore there is no direct data connection between parent and children RefetchContainer, their parent-children relationship is solely because of the UI hierarchy.

So what I am doing in parent RefetchContainer is partial refetch and it works (hope its intentional, not luck). Simplified version:

export default createRefetchContainer(Studio, {
  viewer: graphql.experimental`
    fragment Studio_viewer on Viewer 
     @argumentDefinitions(
         projectId: {type: "String", defaultValue: null},
     ) {
        ...ChildRefetchContainer_viewer
        user {
            id
            name
            ...
            manyOtherThings
            ...
            project (projectId: $projectId) {
                id
                ...MainView_project
            }
        }
    }
  `
},
graphql.experimental`
    query StudioRefetchQuery(
        $projectId: String
    ) {
        viewer {
            user {
                project (projectId: $projectId) {
                    id
                    ...MainView_project
                }
            }
        }        

    }
`);

And child RefetchContainer refetch some other field on viewer. Therefore makes sense if they operate independently and each keeps its own variables. Workaround I could think of to prevent disposing in my case would be wrap the refetchContainer to component which would pass down the relay context, but would keep the variables always same as in my use case I am not interested in any parent variables at all. Maybe it could be option when creating RefetchContainer?

[jardakotesovec]

I use this component as workaround to block variables propagation to RefetchContainer. So basically just wrap RefetchContainer which I want to work independently on parent variables.

import PropTypes from 'prop-types';
import React from 'react';
class RelayRefetchDisposeBlocker extends React.Component {

    constructor(props, context) {

        super(props, context);
        this._variables = {};
        this._relayContext = {
            environment: context.relay.environment,
            variables: this._variables,
        };
    }
    getChildContext() {
        return { relay: this._relayContext };
    }

    componentWillReceiveProps(nextProps, nextContext) {

        if (nextContext.relay.environment !== this.context.relay.environment) {
            this._relayContext = {
                environment: nextContext.relay.environment,
                variables: this._variables
            };
        }
    }

    render() {
        return this.props.children;
    }

};

RelayRefetchDisposeBlocker.contextTypes = {
    relay: PropTypes.object
};

RelayRefetchDisposeBlocker.childContextTypes = {
    relay: PropTypes.object
};

export default RelayRefetchDisposeBlocker;

[sibelius]

@jardakotesovec do you have a snippet of usage of this component?

[jardakotesovec]

@sibelius This should ensure that change in variables which happens in SomeParentRefetchContainer won't propagate to the SomeChildRefetchContainer, which would cause the dispose.

<SomeParentRefetchContainer>
  ...
  <div>
       <RelayRefetchDisposeBlocker>
          <SomeChildRefetchContainer />
       <RelayRefetchDisposeBlocker />
 </div>
</SomeParentRefetchContainer>

[sibelius]

@leebyron @mike-marcacci provide a valid use case here https://github.com/facebook/relay/issues/2237#issue-282565080

it has a demo repository and also a video where you can check the crash

[sibelius]

Relay team is working on fragment ownership model that could probably solve this issue

Added fragment ownership model to relay-runtime: fragments can now point to the query that owns them, which removes reliance on React Context and gives us flexibility to experiment with new apis.

[mrtnzlml]

@sibelius Is there some docs regarding this new ownership concept? I know about it from the commits but didn't understand the idea fully yet... :)

[sibelius]

I think the idea is that if you have more than one QueryRenderer using the same fragment, the fragment will be owned by each QR, so refetching on one QR won't affect the fragment of another QR

[sibelius]

is this fixed?

[jardakotesovec]

@sibelius Unfortunately don't know, I am not using Relay currently in any project, so lost track of this.

[EightArmCode]

We ran into the same problem. Our structure is a parent refetch container which has a child which could ideally also refetch when it is opened. Both the parent and child decide to refetch based on the same condition. If they were side by side, they would not clash, but ultimately if the child is opened they are both unable to resolve. After testing it appears to be a race condition about which refetch invocation will cancel the other, depending upon the timing of opening the child.

Currently looking into passing the dispose() callback to the child to suspend the parent (summary) refetch in favor of the more detailed child refetch, but it's fairly likely that we will just have to query the exhaustive payload at the parent level to workaround this.

Tangentially, we're really interested in the new @refetchable fragment hook, but currently missing a piece in our API to implement it. Wondering/hoping it could solve the issue? If anyone has any insight on this, we'd love to hear it.

[sibelius]

you need to wait for parent finish to avoid race conditions