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

Update to use Ember.Array API... #66

Merged
merged 1 commit into from
Oct 11, 2015
Merged

Update to use Ember.Array API... #66

merged 1 commit into from
Oct 11, 2015

Conversation

lukemelia
Copy link
Collaborator

… so that it will work with ArrayProxy, PromiseArray, etc

Fixes #55

@lukemelia
Copy link
Collaborator Author

@krisselden @mmun @raytiley would love a code review on this

this.set('_items', items);

if (items && items.addObserver) {
items.addObserver('[]', this, this._needsRevalidate);
if (items && items.addEnumerableObserver) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the point of this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the previous code, calling addObserver with a property of '[]'raised on error when used with an ArrayProxy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a bug in ArrayProxy to be honest, also, addEnumerableObserver is soft deprecated and marked as private in 2.0. We can use addArrayObserver but to be honest I was adding a indirection here called dataSource so people could roll their own sparse array as part of the layout bin refactor but I'd like to pull this in for the mean time.

krisselden added a commit that referenced this pull request Oct 11, 2015
Update to use Ember.Array API...
@krisselden krisselden merged commit bf75250 into master Oct 11, 2015
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants