Skip to content
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

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

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

Comments

@rajaravipati
Copy link

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
Copy link
Contributor

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

@rajaravipati
Copy link
Author

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
Copy link
Contributor

Can those properties be accessed via .attr ?

@rajaravipati
Copy link
Author

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

@daffl
Copy link
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
Copy link
Author

@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
Copy link
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 as completed in ef9296b Nov 6, 2012
amcdnl added a commit that referenced this issue Nov 6, 2012
# 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
Projects
None yet
Development

No branches or pull requests

3 participants