Open guyarad opened 6 years ago
I have done a preliminary analysis of the issue. It seems like the client isn't asking the server for the deleted node as part of the node
request, in order to detect the conflicts.
The reason it's not asking for is this line.
However, simply asking for deleted as well won't do the trick, because later when detecting conflicts here, it will fetch the entity from the coreGraph
, but due to weird code here it will simply fetch the base entity, which will in turn resolve the conflicts easily without involving the user, and without even knowing it was deleted, so the end result is local changes are essentially discarded.
Seems like there needs to be a flow that's aware of locally deleted nodes and offers the resolve these conflicts properly.
Nice catch!
but due to weird code here
Yeah, that was a workaround for a bug in some versions of Chrome/Chromium on Linux. I believe it can be removed now since the underlying bug has been fixed in newer versions of Chrome by now (I assume?!).
@tyrasd I'm good like that :)
but it can't be really removed easily. Because if removed, when attempting to get an entity from the coreGraph, it will throw if the entity is deleted locally. That being said, it's probably a good thing because it will identify places which attempted getting an entity without checking if it's there first.
Also, in my opinion, hasEntity
is a little broken for locally deleted entities. It will return false even thought the graph does have the entity, in a sense.
I agree that in order to fix the reported problem, there must be done more than just removing the odd workaround. Maybe it will be necessary to store a visible=false
version for deleted objects (instead of just setting them to undefined
in the graph) in order to fix the issue. :thinking:
Yeah, that was a workaround for a bug in some versions of Chrome/Chromium on Linux. I believe it can be removed now since the underlying bug has been fixed in newer versions of Chrome by now (I assume?!).
yeah I agree - It's been a year since we needed to patch Chrome 58/59.. we can probably remove it.
Also, in my opinion, hasEntity is a little broken for locally deleted entities. It will return false even thought the graph does have the entity, in a sense.
This is the design of how it works. hasEntity
is supposed to return undefined
if the entity has been deleted.
That said, the save.js code can be improved - it already checks the modified objects, and it should check the deleted objects too.
I also have this issue in version 2.12.2 when editing and deleting the same objects. I also saw in source codes that deleted objects are not checked, only modified. Please inform if something changes.
Repro steps:
Actual behavior:
Expected behavior: