can.Model.List doesn't fire the change event on the expando properties #129

Closed
rajaravipati opened this Issue Oct 28, 2012 · 7 comments

Comments

Projects
None yet
3 participants
@rajaravipati

http://donejs.com/docs.html#!can.Model.static.models

{
  count: 15000 //how many total items there might be
  data: [{id: 1, name : "justin"},{id:2, name: "brian"}, ...]
}

In this case, models will return a can.Model.List of instances found in data, but with additional properties as expandos on the list:

The change event should fire for each changed expando property as all the expando properties can be accessed via the .attr method.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Oct 28, 2012

Contributor

Are you changing those properties? I tend not to. Plus, list doesn't support this.

Contributor

justinbmeyer commented Oct 28, 2012

Are you changing those properties? I tend not to. Plus, list doesn't support this.

@rajaravipati

This comment has been minimized.

Show comment
Hide comment
@rajaravipati

rajaravipati Oct 28, 2012

Hi Justin,

I agree with you. But if a property can be accessed via .attr then it can be set too, which means people expect it to be able to use it in their EJS templates for live bindings.

For eg: If i manually did employee.attr('count', 500) it does fire the change event.

Hi Justin,

I agree with you. But if a property can be accessed via .attr then it can be set too, which means people expect it to be able to use it in their EJS templates for live bindings.

For eg: If i manually did employee.attr('count', 500) it does fire the change event.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Oct 31, 2012

Contributor

Can those properties be accessed via .attr ?

Contributor

justinbmeyer commented Oct 31, 2012

Can those properties be accessed via .attr ?

@rajaravipati

This comment has been minimized.

Show comment
Hide comment
@rajaravipati

rajaravipati Oct 31, 2012

YEP. eg: The expando properties can be accessed either via employee.count or employee.attr('count')

YEP. eg: The expando properties can be accessed either via employee.count or employee.attr('count')

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Nov 1, 2012

Contributor

@rajaravipati I'm not sure I understand your problem. This seems to be working:

var Model = can.Model({});
var list = Model.models({
    count : 1000,
    data : [{ id : 1, name : 'first' }, { id : 2, name : 'second' }]
});

console.log(list.attr('count'));
list.bind('change', function() {
    console.log(arguments);
});
list.attr('count', 200);
​

Fiddle

Contributor

daffl commented Nov 1, 2012

@rajaravipati I'm not sure I understand your problem. This seems to be working:

var Model = can.Model({});
var list = Model.models({
    count : 1000,
    data : [{ id : 1, name : 'first' }, { id : 2, name : 'second' }]
});

console.log(list.attr('count'));
list.bind('change', function() {
    console.log(arguments);
});
list.attr('count', 200);
​

Fiddle

@rajaravipati

This comment has been minimized.

Show comment
Hide comment
@rajaravipati

rajaravipati Nov 1, 2012

@daffl You are right & thats what i said in my earlier comment :)
The change event does work if you call it manually like you did in your example - list.attr('count', 200)

But thats not the way the expando properties gets assigned in the source code. Please see line no 732 (res[prop] = val;) in https://github.com/jupiterjs/canjs/blob/master/model/model.js
It does NOT use the attr() method to set the data.

I have created the following http://jsfiddle.net/vAv2t/ to play with. If you click the 'refresh data' button the data array does get updated but not the count property which is updated with a random number every time the findAll method is called. So the EJS template doesn't update due to the change event not firing for each expando property. Check the console.log messages.

@daffl You are right & thats what i said in my earlier comment :)
The change event does work if you call it manually like you did in your example - list.attr('count', 200)

But thats not the way the expando properties gets assigned in the source code. Please see line no 732 (res[prop] = val;) in https://github.com/jupiterjs/canjs/blob/master/model/model.js
It does NOT use the attr() method to set the data.

I have created the following http://jsfiddle.net/vAv2t/ to play with. If you click the 'refresh data' button the data array does get updated but not the count property which is updated with a random number every time the findAll method is called. So the EJS template doesn't update due to the change event not firing for each expando property. Check the console.log messages.

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Nov 1, 2012

Contributor

Ah got it! Problem is, that .models() will return a new model list every time you call it anyway. So what you were thinking of won't work.
You are right with the properties though. I think they should be added using .attr.

Contributor

daffl commented Nov 1, 2012

Ah got it! Problem is, that .models() will return a new model list every time you call it anyway. So what you were thinking of won't work.
You are right with the properties though. I think they should be added using .attr.

@daffl daffl closed this in ef9296b Nov 6, 2012

amcdnl added a commit that referenced this issue Nov 6, 2012

Merge remote-tracking branch 'origin/master' into mustache
# By Eli Morris-Heft (4) and others
# Via Josh Dean (3) and others
* origin/master:
  Undo pull request exposing .abort, due to failing tests.
  Added inlineEJS.html to test issue #108.
  Added a test for #103.
  Use .attr for model list expando properties. Closes #129
  provides Observe.startBatch Observe.stopBatch and Observe.triggerBatch
  Update to Element.NativeEvents to include hashchange and popstate.
  Added a test for indirectly recursive EJS templates. (Ref: Issue #103.)
  Added quoted values to Observe.delegate selectors.
  adding a test for #126
  makes Lists be able to create their associated Observe constructor
  issue #130 - pass jQuery a node list instead of a fragment
  Return the original promise so abort() is not lost.
  add a test for ajaxMethod.abort()
  expose abort() method on Model calls (if exists)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment