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

regression 3.0: ArrayProxy#pushObjects doesn't accept ArrayProxy #16621

Closed
2 tasks
hjdivad opened this issue May 7, 2018 · 6 comments · Fixed by #16853
Closed
2 tasks

regression 3.0: ArrayProxy#pushObjects doesn't accept ArrayProxy #16621

hjdivad opened this issue May 7, 2018 · 6 comments · Fixed by #16853

Comments

@hjdivad
Copy link
Member

hjdivad commented May 7, 2018

#16131 cleaned up a lot of Enumerable and Array mixin code.

However it also changed the assertion in pushObjects from checking for Enumerable or Array to only checking Array

This assertion now throws when calling hasMany.pushObjects(otherHasManyOrRecordArray) even though ember-data's ManyArray can accept these objects.

After going over this with @rwjblue I think the path forward is

  • Drop the assertion in pushObjects entirely
  • docs: Implementations have to assert their arguments in replace (which is the method implementations of MutableArray are expected to implement)

NativeArray already does assert in replace and ArrayProxy doesn't need to as it delegates to content.

@rwjblue
Copy link
Member

rwjblue commented May 7, 2018

Thanks for writing this up, @hjdivad! Agreed on the path forward...

@mmun - any objections?

@mmun
Copy link
Member

mmun commented May 7, 2018

@rwjblue Nope, this seems good.

It would also be useful if we could mark all the methods that are derived from replace as "final" so that people don't override them.

@runspired
Copy link
Contributor

using Ember.isArray instead of Array.isArray is sufficient here.

@rwjblue
Copy link
Member

rwjblue commented May 8, 2018

@runspired - Yes, that would work right, but I believe that it muddies the waters a bit more than the proposed fix...

@devdemi
Copy link

devdemi commented Jul 20, 2018

I have same problem I can't use pushObjects passing RecordArray.

@nlfurniss
Copy link
Contributor

@devdemi you can get around the issue currently by called toArray on the Enumerable you're trying to push in

myCollection.pushObjects(newElements.get('elements').toArray());

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 a pull request may close this issue.

6 participants