New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Modern] NODE_DELETE does not remove node from connections #2155

Closed
dminkovsky opened this Issue Oct 18, 2017 · 8 comments

Comments

Projects
None yet
5 participants
@dminkovsky
Copy link
Contributor

dminkovsky commented Oct 18, 2017

NODE_DELETE deletes a node from the store, but does not delete corresponding edges from connections that contain the node. The result is after a NODE_DELETE, pagination containers that render a connection that had this node will receive connections props with the an edge that has a cursor but null a node.

The documentation says NODE_DELETE:

Given a deletedIDFieldName, Relay will remove the node(s) from the connection.

setRelayModernMutationConfigs.js seems to verify this as the intended behavior.

Wouldn't it make sense for NODE_DELETE to delete from ranges that contain this node, too? Would that be possible given the new runtime? Is the correct solution to explicitly add a RANGE_DELETE config in addition NODE_DELETE to remove from the connection? Should the docs be updated?

@dminkovsky

This comment has been minimized.

Copy link
Contributor

dminkovsky commented Oct 18, 2017

Reading through RelayConnectionHandler, it seems like this would be pretty complicated. You'd have to look through all connections that might possibly have this node?

@bharathmuraleedharan

This comment has been minimized.

Copy link

bharathmuraleedharan commented Dec 7, 2017

Basically NODE_DELETE is useless alone, it should be used along with RANGE_DELETE to avoid npes!

@DavidHe1127

This comment has been minimized.

Copy link

DavidHe1127 commented Jan 7, 2018

@dminkovsky

have you got this issue resolved? I have the same problem and no idea what to do with null node

@dminkovsky

This comment has been minimized.

Copy link
Contributor

dminkovsky commented Jan 11, 2018

@DavidHe1127 nope, I never resolved it beyond moving from configs to updaters, which I think is the intended way in Relay Modern. As @bharathmuraleedharan points out, you can write an updater and target the connection you're looking to remove from.

I haven't look at this in a while, so maybe something changed: but if you're trying to remove a node from all connections after you delete a node, short of knowing a priori which connections it belonged to and removing that node from those connections, it appears there is no easy way to do that.

@jamesone

This comment has been minimized.

Copy link

jamesone commented Aug 16, 2018

Confirmed that I see the same issue - An edge remains with the following contents:

{
cursor: 'xxx',
node: null,
}

This happens using the following config:

const configs = [
  {
    type: 'NODE_DELETE',
    deletedIDFieldName: 'id',
  }
];

const optimisticResponse = {
  deleteUser: {
    id: id,
  },
};

commitMutation({
  mutation,
  variables,
  optimisticResponse,
  configs,  
  onCompleted: res => resolve(res),
  onError: err => reject(err),
});
@taion

This comment has been minimized.

Copy link
Contributor

taion commented Aug 31, 2018

You can add a RANGE_DELETE config to clean up edges from any relevant connections.

@dminkovsky

This comment has been minimized.

Copy link
Contributor

dminkovsky commented Sep 1, 2018

Yeah, that's my thinking on this too now. There's no way to remove from all connections the node might be in unless you know the connections, but given the typical GC/fetch data flows I've experienced in Relay, that's okay.

@dminkovsky dminkovsky closed this Sep 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment