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

Get converters and .attr working the right way with nested objects #264

Closed
moschel opened this Issue Feb 1, 2013 · 4 comments

Comments

Projects
None yet
2 participants
@moschel
Contributor

moschel commented Feb 1, 2013

The expected behavior of .attr when passed a nested object is as follows:

myobj.attr('prop', obj);

  • if obj is a can.Observe, we should replace whatever is currently in prop with obj
  • if obj is an object, its properties should be merged with myobj.attr('prop')
  • the converter for 'prop' should then be called, once, with the merged or replaced property

This is not how it currently works, currently converters are called first, then _set or .attr is called, and converter is called again in _set. Fixing this logic will likely fix #208 and #174.

@ghost ghost assigned moschel Feb 1, 2013

@moschel

This comment has been minimized.

Show comment
Hide comment
@moschel

moschel Feb 2, 2013

Contributor

f9896d9 is where this went wrong. After fixing the logic, the question is what to do about this case:

  can.Model('NestedAttrTest.Task', {
      attributes : {
        owner : "NestedAttrTest.User.model"
      }
    }, {});
    var michael = NestedAttrTest.User.model({ id : 17, name : 'Michael'});
    var amy = NestedAttrTest.User.model({ id : 29, name : 'Amy'});
    var task = NestedAttrTest.Task.model({
      id : 1,
      name : "Do it!",
      owner : {id : 17}
    });
    task.attr({ owner : { id : 29, name : 'Amy'}});

The way its currently written, these owner attributes will be merged with what's currently in the owner observe. This means the actual data in the model store is overwritten, including the id and name.

In this case we need to know about the fact that owner is a model instance and not overwrite.

Contributor

moschel commented Feb 2, 2013

f9896d9 is where this went wrong. After fixing the logic, the question is what to do about this case:

  can.Model('NestedAttrTest.Task', {
      attributes : {
        owner : "NestedAttrTest.User.model"
      }
    }, {});
    var michael = NestedAttrTest.User.model({ id : 17, name : 'Michael'});
    var amy = NestedAttrTest.User.model({ id : 29, name : 'Amy'});
    var task = NestedAttrTest.Task.model({
      id : 1,
      name : "Do it!",
      owner : {id : 17}
    });
    task.attr({ owner : { id : 29, name : 'Amy'}});

The way its currently written, these owner attributes will be merged with what's currently in the owner observe. This means the actual data in the model store is overwritten, including the id and name.

In this case we need to know about the fact that owner is a model instance and not overwrite.

@moschel

This comment has been minimized.

Show comment
Hide comment
@moschel

moschel Feb 2, 2013

Contributor

The above commit gets all the issues Josh was trying to fix working, but avoids calling __convert too early. I'm unsure if this is the right way to fix the above issue:

if( canMakeObserve(curVal) && curVal instanceof can.Model && canMakeObserve(newVal) ) {
  call _set
}

My logic is if the curVal is a model and the new one looks like it should be data for this model, call _set to let the converter handle it. I could probably come up with some situations where this wouldn't work, like when there is some very weird custom converter that isn't .model or .models on this instance and they want the data to be merged into the existing model. Thoughts?

Contributor

moschel commented Feb 2, 2013

The above commit gets all the issues Josh was trying to fix working, but avoids calling __convert too early. I'm unsure if this is the right way to fix the above issue:

if( canMakeObserve(curVal) && curVal instanceof can.Model && canMakeObserve(newVal) ) {
  call _set
}

My logic is if the curVal is a model and the new one looks like it should be data for this model, call _set to let the converter handle it. I could probably come up with some situations where this wouldn't work, like when there is some very weird custom converter that isn't .model or .models on this instance and they want the data to be merged into the existing model. Thoughts?

@moschel

This comment has been minimized.

Show comment
Hide comment
@moschel

moschel Feb 2, 2013

Contributor

updated to not reference can.Model here: ae6b39c

Contributor

moschel commented Feb 2, 2013

updated to not reference can.Model here: ae6b39c

@moschel

This comment has been minimized.

Show comment
Hide comment
@moschel

moschel Feb 3, 2013

Contributor
Contributor

moschel commented Feb 3, 2013

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