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

Confusing behavior when modifying data and using fall-through-cache #314

Closed phillipskevin closed 7 years ago

phillipskevin commented 7 years ago

Modifying data returned by get() or getList() before it is updated by the fall-through-cache can cause very confusing behavior. This can be seen by changing the data in the .then() of the Promise returned. Here is an example of what I mean:

var VM = can.DefineMap.extend({
  message: can.DefineMap,

  init: function() {
    var vm = this;
    Message.get({ id: 16 }).then(function(message) {
      vm.message = message;
      vm.message.body += '!!!!';      
    });
  }
});

The message.body will be updated to include the !!!! momentarily, but then will be reverted back to not have the !!!! once the instance is updated here.

Here is a JSBin if you want to try it out: https://jsbin.com/jucuvezoyo/1/edit?html,js,output.

We should either give a warning (not sure how we would do this) or perhaps diff instanceData and instanceData2 and update the instance with the differences.

justinbmeyer commented 7 years ago

Yeah, I don't think this is easily solvable in a general way. Merge conflicts are one of the hardest things to solve. Everyone has their own pattern.

Generally speaking, the response from the server is the latest dat, which you would want to overwrite.

Cloneable can help with this.

Sent from my iPhone

On Jun 28, 2017, at 8:40 AM, Kevin Phillips notifications@github.com wrote:

Modifying data returned by get() or getList() before it is updated by the fall-through-cache can cause very confusing behavior. This can be seen by changing the data in the .then() of the Promise returned. Here is an example of what I mean:

var VM = can.DefineMap.extend({ message: can.DefineMap,

init: function() { var vm = this; Message.get({ id: 16 }).then(function(message) { vm.message = message; vm.message.body += '!!!!';
}); } }); The message.body will be updated to include the !!!! momentarily, but then will be reverted back to not have the !!!! once the instance is updated here.

Here is a JSBin if you want to try it out: https://jsbin.com/jucuvezoyo/1/edit?html,js,output.

We should either give a warning (not sure how we would do this) or perhaps diff instanceData and instanceData2 and update the instance with the differences.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

nlundquist commented 7 years ago

I agree with @justinbmeyer, avoiding this unintended data change should be the responsibility of the application.

can-connect-clonable may be able to help with this but it currently appears to be bugged. I've modified @phillipskevin JSBin to use clonable and the cloned view model doesn't update the template as expected: https://efficient-territory.glitch.me/ (source: https://glitch.com/edit/#!/efficient-territory?path=index.html:1:0)

In the example above the message.body property of the view model is printed to the console, but it doesn't match the message body rendered by the template. Changing the VM message body via the console testVM.message.body = 'test'; has no effect either. Even though the cloned instance is used as the VM, something is preventing it from binding correctly to the template.

I'll investigate further and open a bug for can-connect-clonable.

nlundquist commented 7 years ago

I've opened canjs/can-connect-cloneable#13 to capture the binding issue when using clonable.

I'm closing this issue as for the foreseeable future we won't be building a solution for this into can-connect itself and will instead require application devs to use instance clones to avoid unintended updates like this.