RANGE_ADD - Better warning messages when new edge is missing #574

xuorig · 2015-11-10T14:40:35Z

Possible improvement for #542

Lot's of issues and misunderstanding with that one, range add mutations can be confusing with the current warning messages.

Better solution as @steveluscher said:

When relay does not query the new edge because it was not included in the intersection of tracked and fat queries, show a different warning saying it's a client-side problem ( Connection was never used in the app )
If the edge was queried but is not in the payload, explain it's a server side problem.

In handleRangeAdd, if the edge is missing from the payload, check if the operation RelayQueryMutation contained the newEdge field. If it didn't, it means the new edge was not included because it wasn't in the intersection of the tracked/fat query. If it did, it means the server should've returned that field and didn't.

I've added a function getNodeByFieldName to check if the operation contained the field to get the new edge. Not sure if it is the best way to check that, let me know.

The actual warning messages probably need some work, let me know what you think would be the best messages for these cases.

josephsavona · 2015-11-10T15:52:12Z

src/traversal/writeRelayUpdatePayload.js

-      'update the `RANGE_ADD` mutation config?',
-      config.edgeName
-    );
+    if (operation.getNodeByFieldName(config.edgeName)) {


use operation.getFieldByStorageKey(config.edgeName)

ah of course. Updated the PR!

josephsavona · 2015-11-10T15:56:06Z

cc @steveluscher @kassens

devknoll · 2015-11-10T18:34:59Z

Thank you!!! ❤️

xuorig · 2015-11-18T18:18:12Z

@josephsavona @kassens any comments on this ? Would be helpful to add that in 👯

steveluscher · 2015-12-08T01:54:32Z

Thanks for your patience on this. It's taken us a long time to get to it, I know. Here's what we're thinking.

To recap, these are two distinct cases.

If the query includes the field but the response payload doesn't, it indicates a server error (the server should have returned null if it can't fetch a field).
If the query doesn't include the field, it indicates that the range configs do not account for the tracked fields. Since this is a problem with the query rather than the response payload, what do you think about moving this warning out of the writer and into RelayMutationQuery, making it clear that the problem is range behaviors in RANGE_ADD not matching up with what the application is actually querying for by way of a helpful and well-written error message?

xuorig · 2015-12-17T05:10:02Z

@steveluscher sorry for the wait, (finals week 😪) I have updated the PR with what you have recommended. I make the check in buildFragmentForEdgeInsertion. If the newEdge is not present in mutatedFields after it's been populated, we can assume rangeBehaviors was not matching the trackedConnection and we can warn.

Should this be an invariant or keep it as a warn ? Let me know if this is what you had in mind!

josephsavona · 2015-12-17T16:45:36Z

src/mutation/RelayMutationQuery.js

@@ -242,6 +243,21 @@ var RelayMutationQuery = {
        }
      }
    }
+
+    // Warn if the newly created edge was not included in mutatedFields
+    const edgeField = mutatedFields.find(field => {


Instead, how about moving this to an else clause at line 225? Note that mutatedEdgeFields is only empty if no range behaviors matched and the new edge field would be missing.

josephsavona · 2015-12-17T16:47:11Z

Thanks for updating this! We should also remove the warning in writeRelayUpdatePayload (and just return if appropriate).

xuorig · 2015-12-17T16:50:59Z

Shouldn't we keep the current warning in the case where the server is not returning the newEdge ?

josephsavona · 2015-12-17T16:52:19Z

We should keep warning if the server doesn't return the edge, but only if we asked for the edge and didn't get it. The warning currently still occurs even if no range behaviors matched.

xuorig · 2015-12-17T16:56:14Z

Ah you're right! Updated the PR to address your comments.

josephsavona · 2015-12-17T17:01:13Z

src/mutation/__tests__/RelayMutationQuery-test.js

+        parentID: '123',
+        edgeName: 'feedbackCommentEdge',
+        parentName: 'feedback',
+        rangeBehaviors,


nit: i suspect you borrowed this from other test cases in this file, but it would make the test more self-contained and readable if we were explicit about the range behaviors, e.g rangeBehaviors: {}.

Much clearer that way!

josephsavona · 2015-12-17T17:08:44Z

A few more nits, but otherwise this looks great :-)

steveluscher · 2016-01-06T00:20:26Z

Thanks for your patience, @xuorig. We just went over this internally, and I'd like to share a summary of our discussion. I wrote:

Let me try to work through this to make sure I understand it.

Client devs have the option of configuring a RANGE_ADD config or not. If they do configure one, it's likely because they expect it to be of use.

Now, if you've specified a RANGE_ADD config, there are tracked connections that match, but none of them match your rangeBehaviors, it's because you have a connection in your app with arguments that you haven't accounted for in your RANGE_ADD config. This is the aim of the warning, to say “hey… we found at least one tracked connection for that parentID/connectionName combination, but we couldn't find an associated rangeBehavior – did you forget to configure one?”

@yungsters: …this warning … can be triggered [if] the key does not have a range behavior defined.

Interesting. To fallback by refetching the whole connection is good (and what the code does now if you don't have a matching range behavior) but one of the nice things about RANGE_ADD configs is to prevent refetching and to enable optimistic mutations. When you miss a rangeBehavior, you lose all of that without warning.

What if we changed the warning to indicate that the entire connection has been refetched and that the developer can't expect optimistic mutations to work, with a little bit about how to write a more efficient and optimism-compatible mutation. Something like:

“Relay.Mutation: Since the connection ships(orderby:"latest") of the field faction with id 123 matched none of the rangeBehaviors specified in your RANGE_ADD config, the entire connection has been refetched. Configure a range behavior for this connection to fetch only the new edge and to enable optimistic mutations. See http://… for more information.”

The only problem here is that there's no way for the developer to squelch this warning if their desired behavior was to refetch the connection. Thoughts, @yuzhi @xuorig @yungsters?

yungsters · 2016-01-06T01:12:22Z

I discussed with @yuzhi a bit. Currently, setting a range behavior configuration to null means the range should not be refetched at all. I think we should change this to IGNORE instead of null.

We can add a REFETCH or ALL constant to squelch the warning.

xuorig · 2016-01-06T14:37:00Z

Correct me if I'm wrong but If the desired behavior was to refetch the connection, why would there be a RANGE_ADD then ? wouldn't you use something like REQUIRED_CHILDREN instead ?

We can add a REFETCH or ALL constant to squelch the warning.

That would definitly help remove any ambiguity about the behavior.

yungsters · 2016-01-07T03:08:31Z

A RANGE_ADD may be an APPEND for some but REFETCH for others (e.g. if a particular argument causes sorting that can only be determined by the server).

xuorig · 2016-01-10T23:02:34Z

@steveluscher @yungsters I propose that we change the warning message to something like @steveluscher proposed, where we clearly state which connection wasn't found in the rangeBehaviors.

In another PR we can address the REFETCH / ALL and IGNORE behaviors ? I would gladly work on a solution for this too.

…hes no range behavior

xuorig · 2016-01-11T03:12:50Z

I have modified the PR with the message @steveluscher proposed. I think this might be a good first step to clear up the small confusion people have with RANGE_ADD mutations.

yungsters · 2016-03-07T22:45:36Z

@facebook-github-bot import

facebook-github-bot · 2016-03-07T23:13:25Z

Thanks for importing. If you are an FB employee go to Phabricator to review.

Summary:As discussed in #574, #542 Introduce 2 new range behaviors: - `REFETCH`: Will refetch the entire connection (to squelch the warning when no `rangeBehavior` matches the tracked connection) - `IGNORE`: Replaces using null, means the range should not be refetched at all. I've deprecated `null`, maybe we don't need to, what do you think ? yungsters steveluscher let me what you think! ? Closes #945 Reviewed By: josephsavona Differential Revision: D3117256 Pulled By: wincent fb-gh-sync-id: e7b5ab2fe6beebcd53f400a62634a7cd5aae383b fbshipit-source-id: e7b5ab2fe6beebcd53f400a62634a7cd5aae383b

facebook-github-bot added the CLA Signed label Nov 10, 2015

josephsavona reviewed Nov 10, 2015
View reviewed changes

xuorig force-pushed the better-range-add-warnings branch from db91128 to e8da871 Compare November 10, 2015 16:02

xuorig force-pushed the better-range-add-warnings branch from e8da871 to 11b82be Compare December 17, 2015 05:06

josephsavona reviewed Dec 17, 2015
View reviewed changes

xuorig force-pushed the better-range-add-warnings branch from 11b82be to 65d9b78 Compare December 17, 2015 16:56

xuorig force-pushed the better-range-add-warnings branch from 65d9b78 to 08300ad Compare December 17, 2015 16:57

josephsavona reviewed Dec 17, 2015
View reviewed changes

xuorig force-pushed the better-range-add-warnings branch from 08300ad to 6a5f0c3 Compare December 17, 2015 17:15

xuorig force-pushed the better-range-add-warnings branch from 6a5f0c3 to 8cf11df Compare January 11, 2016 03:10

Warn that connection will be refetched if the tracked connection matc…

8a7a26c

…hes no range behavior

xuorig force-pushed the better-range-add-warnings branch from 8cf11df to 8a7a26c Compare January 11, 2016 03:11

yungsters assigned wincent and yungsters and unassigned wincent Mar 1, 2016

ghost closed this in cde735b Mar 9, 2016

xuorig deleted the better-range-add-warnings branch March 9, 2016 03:11

xuorig mentioned this pull request Mar 11, 2016

Add REFETCH and IGNORE rangeBehaviors #945

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RANGE_ADD - Better warning messages when new edge is missing #574

RANGE_ADD - Better warning messages when new edge is missing #574

xuorig commented Nov 10, 2015

josephsavona Nov 10, 2015

xuorig Nov 10, 2015

josephsavona commented Nov 10, 2015

devknoll commented Nov 10, 2015

xuorig commented Nov 18, 2015

steveluscher commented Dec 8, 2015

xuorig commented Dec 17, 2015

josephsavona Dec 17, 2015

josephsavona commented Dec 17, 2015

xuorig commented Dec 17, 2015

josephsavona commented Dec 17, 2015

xuorig commented Dec 17, 2015

josephsavona Dec 17, 2015

xuorig Dec 17, 2015

josephsavona commented Dec 17, 2015

steveluscher commented Jan 6, 2016

yungsters commented Jan 6, 2016

xuorig commented Jan 6, 2016

yungsters commented Jan 7, 2016

xuorig commented Jan 10, 2016

xuorig commented Jan 11, 2016

yungsters commented Mar 7, 2016

facebook-github-bot commented Mar 7, 2016

RANGE_ADD - Better warning messages when new edge is missing #574

RANGE_ADD - Better warning messages when new edge is missing #574

Conversation

xuorig commented Nov 10, 2015

josephsavona Nov 10, 2015

Choose a reason for hiding this comment

xuorig Nov 10, 2015

Choose a reason for hiding this comment

josephsavona commented Nov 10, 2015

devknoll commented Nov 10, 2015

xuorig commented Nov 18, 2015

steveluscher commented Dec 8, 2015

xuorig commented Dec 17, 2015

josephsavona Dec 17, 2015

Choose a reason for hiding this comment

josephsavona commented Dec 17, 2015

xuorig commented Dec 17, 2015

josephsavona commented Dec 17, 2015

xuorig commented Dec 17, 2015

josephsavona Dec 17, 2015

Choose a reason for hiding this comment

xuorig Dec 17, 2015

Choose a reason for hiding this comment

josephsavona commented Dec 17, 2015

steveluscher commented Jan 6, 2016

yungsters commented Jan 6, 2016

xuorig commented Jan 6, 2016

yungsters commented Jan 7, 2016

xuorig commented Jan 10, 2016

xuorig commented Jan 11, 2016

yungsters commented Mar 7, 2016

facebook-github-bot commented Mar 7, 2016