canjs / can-connect

Model layer utilities for every JavaScript framework! Assemble real-time, high performance, restful data connections.
https://canjs.com/doc/can-connect.html
MIT License
29 stars 16 forks source link

DefineList on 'length' event listener isn't triggered on item destroyed #365

Closed ivospinheiro closed 6 years ago

ivospinheiro commented 6 years ago

How often can you reproduce it?

Description:

Using can-connect baseMap (includes the behavior real-time) when destroying an item on a DefineList, the 'length' event listener isn't triggered when the item is destroyed and also the view doesn't update if we are showing only the DefineList length.

Steps to reproduce:

Here is a JSBin Example: https://jsbin.com/wivekuwuza/2/edit?html,js,console,output

Expected results:

When running the demo it was expected a descendant count from 3 to 0 and the "todos.on('length', ...)" to be triggered whenever an item is removed.

Actual results:

Only a count down from 3 to 2 is verified, probably because when destroying the item with id '7' an object is returned.

Environment:

Software Version
can-connect version latest
Browser chrome
Operating system Linux
BigAB commented 6 years ago

This seems like it may be more about how realtime handles the response from an 202 No Content DELETE response (or other non identifying entity response), because if you change the example to return an object with an id property equal to the resource you deleted, everything seems to work as expected: https://jsbin.com/pelekal/edit?js,output

Looking in to how 202 No Content (and non-identifying) responses are handled now.

BigAB commented 6 years ago

So I think the crux of everything is here: https://github.com/canjs/can-connect/blob/master/real-time/real-time.js#L319-L324

So params are from the instance, and props are the response from the server.

it uses the id from either props or params, and checks the instance store for an instance with that id. The only way that instance store is going to have a matching instance is if something was subscribing directly to the instance, binding on the list is not enough.

If there is no instance, it takes what was returned from the server, hydrates an instance out of that, and deletes it, which causes the a search through all the lists in the liststore with a matching id, and those remove themselves from the list, and the thing updates...

Which leads us to the 2 conditions this issue has already identified, you either need to be subscribed directly to the instance being deleted somewhere (length of list is not enough), or the response has to identify the instance, with the id from the algebra.

I will now look into the consequences of using the ID from params to make an instance just for deleting.

these comments are more about documenting my journey through fixing this, I am not asking for a reply or anything

BigAB commented 6 years ago

Okay, so the real problem is that when destroying an instance, the instance really should be in the instanceStore for all the correct destroy callbacks to function correctly.

The real fix for this, is for constructor/store/store.js's destroy method to temporarily add it to the instance store using addInstanceReference (and subsequently deleteInstanceReference) just like the save method right above it does.

I have tested this, and it fixes this and my own demo.

When I went to write a test, I discovered the tests are not running correctly due to an error: You can't have two versions of can-dom-data-state, check your dependencies. So, I'll look into tracking that down, getting the tests to run, adding a test that covers this demo/behaviour, and then see about making a patch release with this in.

chasenlehara commented 6 years ago

@BigAB FYI @imaustink has been updating our packages that depend on can-dom-data-state.