Relay: NODE_DELETE does not update cached connections as expected?

[NevilleS]

Hello there, I noticed some unexpected behaviour here and I'm not sure whether it's an issue or I'm just misunderstanding the API. I'll try to be as specific as I can, although I don't know of a good way to set up a shareable test case (sorry).

Version

This is using react-relay 0.6.0.

Issue Description

I have a component that renders a list of Groups that an Item belongs to. There is a many-to-many relationship between Items and Groups so it does this by querying a group_items connection nested under the Item itself. I then have a mutation that removes an Item from a specific Group. Right now, the issue I'm seeing is that when I execute the remove mutation, the list of Groups is not updated, so the UI becomes out of date. A full page refresh correctly renders the new data, which means I'm doing something wrong in my mutation.

The fragments are:

fragment on Item {
  id,
  group_items(first: 10) {
    edges {
      node {
        group {
          id,
          name
        }
      }
    }
  }     
}

...the mutation's fat query...:

getFatQuery() {
  return Relay.QL `
    fragment on RemoveItemFromGroupMutationPayload {
      deletedGroupItemId,
      item { group_items }
    }
  `
}

...and the mutation's configs...:

getConfigs() {
  return [{
    type: "NODE_DELETE",
    parentName: "item",
    parentID: this.props.item.id,
    connectionName: "group_items",
    deletedIDFieldName: "deletedGroupItemId"
  }]
}

Expected Behaviour

After running the mutation, the group_item node is destroyed on the server, and the group_items connection is updated & re-rendered on the client, with the specified node deleted.

Actual Behaviour

The server updates correctly, but the group_items connection is not updated, and the deleted node is still available.

Workaround

After some investigation I found that if I made the fat query more specific, the connection would update correctly:

getFatQuery() {
  return Relay.QL `
    fragment on RemoveItemFromGroupMutationPayload {
      deletedGroupItemId,
      item {
        group_items(first: 10) {
          edges
        }
      }
    }
  `
}

Similar issues

Note that this issue is similar to #611 but the workaround given there (provide a FIELDS_CHANGE config to specify the Item that is being updated) has no effect in my application. The only way to get the tracked connection to update is to provide a more specific fat query as I specified above in the workaround.

Any ideas?

[NevilleS]

Aha! :tada:

The more I thought about this, the more I felt that this was too fundamentally "broken" to be an actual Relay bug. After debugging a bit further, I realized that the deletedGroupItemId was the _wrong_ ID; it was a database ID, not a Relay GUID. Therefore the NODE_DELETE was, essentially, silently failing to find a node that matched the id it was given.

I suppose the workaround worked because I was so specific that Relay elected to fetch the entire group_items connection, and noticed there was a node missing.

It'd be nice if the NODE_DELETE had complained to the console in some way so I'd know to take a closer look at the IDs, but in the end this was definitely user error. Sorry for the noise :+1:

[josephsavona]

Ah, that makes sense. Thank you for the detailed write up, and detailed follow up. We should definitely warn if the deleted of field doesn't match up in order to help catch these cases. If you're interested we'd be happy to accept a PR (take a look at writeRelayUpdatePayload).

[NevilleS]

Sounds good, I'll take a look. Happy to help out :+1:

[NevilleS]

OK @josephsavona I took a quick look at writeRelayUpdatePayload to see how I might go about adding a warning for this. It seems like it'd be relatively trivial to test the record status in handleNodeDelete just before the deleteRecrod method is invoked (i.e. lines 128 & 131 here). Alternatively, I could throw a warning in the existing check within deleteRecord although that seems a bit too "low-level" to trigger warnings to me since it is used in lots of other contexts (but it could be done here)

Both of those implementations would be trivial, but my question is whether or not a warning whenever the given node ID's status is RelayRecordState.NONEXISTENT is a fair warning, since the way it is written now it just silently skips the operation assuming the record had already been deleted. I get the sense that it might be annoying to users to see this warning for mutations when they weren't explicitly tracking the node already, which would be common...

In my case, a more useful warning would be something like "The given node ID doesn't appear to be a Relay GUID; are you sure you didn't mess that up?". So I'd rather add a check like isValidRecordID to the mutation code to guard against that kind of user error... but I'm not sure it's possible. I think Relay GUIDs might be opaque to Relay, but I'm not sure since I haven't messed with the source code much before.

What do you think? Would a warning for NONEXISTANT records be annoying? Alternatively, is there a way to validate a record ID?

[NevilleS]

OK, hopefully you're enjoying a holiday break, but wanted to update this.

I poked around the Relay source code quite a bit over the last few days and there's definitely no way to "validate" that a record ID is valid, so that won't work. The question remains whether or not triggering a warning when a NODE_DELETE is attempted makes sense. I imagine that in the majority of use cases, the node being deleted should be in the Relay store already. That's probably because most node delete operations are triggered from a component and use the node ID from the props (e.g. deleting a comment from that comment's component). In those cases it'd be useful to trigger a warning saying "Hey, we didn't actually delete a node, FYI". But in other use cases you'll be deleting something that isn't tracked locally (e.g. a comment you haven't rendered yet) so the warning would be confusing.

Since there's not really a good way to selectively warn users about this, I'm not so sure it's a good idea. I don't like triggering warnings that people learn to ignore...

On another note, I did notice today that I was specifying parentId instead of parentID for a mutation, which is more clearly a user error... :smile:

[Rodeoclash]

I just ran into this bug, stupid oversight. Perhaps a warning that the id was not found and outputting what the id was that was searched for would help catch this.

[sushantdhiman]

Just ran into this issue as well, some sort of warning can save a few hours for next guy :laughing:

[dpehrson]

This has been driving me crazy for a week, Neville's comment made me check my code and sure enough, the deleted item id was not being encoded. Problem solved. A warning would certainly have helped here. Thanks!

[josephsavona]

This is a good idea. Anyone want to send a PR? :-)

[NevilleS]

Unfortunately a warning for a missing ID would probably fire a lot of false positives, since NODE_DELETE will commonly be configured even if the node doesn't necessarily exist.

For example let's pretend we've built a blog app with Relay with Posts and Favourites. Favourites are nodes that connect Users to Posts with some additional metadata, yada yada (pretty common "join" model of some kind).

So you can imagine a RemovePostMutation that returns both the deletedPostID and deletedFavouriteID to clean up both posts and favourites for the current user. The developer will have configured two NODE_DELETE mutation configs to handle the case where the post exists (obviously) but also the case where the user may have a Favourite referencing this post (optional).

When the mutation executes it'll query for the deletedFavouriteID regardless of whether or not the associated node exists locally, and then execute the handleNodeDelete logic to remove that node. If the local graph has queried for the Favourite node already, it'll delete it. If it hasn't been queried yet, it'll just silently ignore the delete. I think this is the right behaviour.

So with that said I'm not sure there's a great way to just do a warning like NODE_DELETE failed to find node with ID "eW91IGNhdWdodCBtZSwgdGhpcyBpc24ndCBhIHJlYWwgUmVsYXkgSUQ=", since it'll be 100% correct behaviour to not have the node in plenty of cases...

Maybe only show the warning when the mutation debugging is enabled?