Fetch missing client edge server queries discovered in nested fragments

[captbaritone]

Summary:
Initially reported here: #4882

Fetching of client-edge-to-server generated queries is handled by detecting missing data in Relay Reader and Relay Reader maintaining a stack of client-edge-to-server fields. As we traverse into such fields we push information about that query onto the stack, and as we exit such fields we pop values off the stack.

In the case of fragments spread into selections where we have such a stack built up, we propagate it via a hidden __clientEdgeTraversalPath field on fragment keys.

This hidden property means that any fragment located somewhere underneath a client edge to server field might encounter missing data and need to trigger the fetch of that server query.

For example, if the selection includes only fragment spreads, or some other query had fetched all the fields that were read directly in the fragment which read the actual resolver field the fragment with the resolver field would render fine and we wouldn't discover we were missing data until we tried to read a child fragment. In theory that could be many fragments away. E.g. if I had a field my_client_edge_to_server that returned a User, any fragment spread into any selection nested under that edge (including my_client_edge_to_server.best_friend.favorite_sports_team.coach.profile_picture could technically uncover missing data in some totally generic ProfilePicture fragment.

However, we overlooked that possibility in D36684124 where we added an optimization which tries to avoid the overhead of these additional hooks in the common case by only checking for missing client edge data in the case that the fragment itself contains at least one client edge field (as computed by the compiler).

As the example above demonstrates (and the test case in this diff also demonstrates) that turns out to not actually be safe.

This removes the optimization to allow the correct behavior to work. However, this adds a useMemo and useEffect to each useFragment, which may have some perf implications. Putting this behind a feature flag to let us ship this especially for folks who are blocked by it, but also so we can consider perf concerns before rolling out internally.

I'll invert the feature flag default in a future diff once we've configured it internally.

Differential Revision: D74593372

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D74593372

[jonreading81]

Thanks for this @captbaritone , Ive patched our version of relay and this has fixed the majority of issues however its still failing when we are using Derived fields for the server edges when using readFragment.

For example:

        const { streamInfo } = readFragment(
            graphql`
                fragment audioUrlCloudcastFragment on Cloudcast
                {
                    streamInfo {
                        url
                    }
                }
            `,
            key,
        );

This is still missing the data, any idea what needs to update for this using your proposed approach?

[captbaritone]

Thanks for trying it out and identifying this additional case. Could you clarify which filed is the client edge to server in this case?

[captbaritone]

@jonreading81 any chance you could create a repro unit test similar to this demonstrating the resolver issue you are seeing? If not, just providing more details might be able to help me construct one.

[captbaritone]

(Mostly a note for myself): I wonder if we are not correctly setting CLIENT_EDGE_TRAVERSAL_PATH here.

Note how we set it here.

[captbaritone]

Nevermind, I think I have a repro of the issue you are seeing, and I think the note to myself above is likely the root cause. I'll update this pr.

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D74593372

[captbaritone]

@jonreading81 I think this is the fix for the resolver issue you are seeing. Do you want to try patching that in as a well and see if it resolves the issue? I intend to land that as a separate PR since it is not strictly dependent on this one

[jonreading81]

Thanks @captbaritone , I have tested this and it fixes the issue. Thanks for your quick response on this.

[facebook-github-bot]

This pull request has been merged in 611f6d9.

[captbaritone]

@jonreading81 This should be available on NPM in the relay packages react-relay@0.0.0-main-611f6d9c, relay-runtime@0.0.0-main-611f6d9c etc.

You'll need to set the feature flag.

RelayFeatureFlags.CHECK_ALL_FRAGMENTS_FOR_MISSING_CLIENT_EDGES = true;