pushObjects() doesn't work when the argument is an ArrayProxy

[bendemboski]

Using Ember 1.8.1 on Chrome 39.0.2171.71, execute the following command in the browser console:

[].pushObjects(Ember.ArrayProxy.create({ content: [ 1, 2, 3 ] }))

Since an ArrayProxy is an enumerable, the return value should be [ 1, 2, 3 ], but instead it is an array containing a single object, which is the ArrayProxy. You can verify this by running:

[].pushObjects(Ember.ArrayProxy.create({ content: [ 1, 2, 3 ] }))[0].constructor

and seeing that the result is Ember.ArrayProxy.

I believe the bug is caused by https://github.com/emberjs/ember.js/blob/v1.8.1/packages/ember-metal/lib/enumerable_utils.js#L134. objects is the array proxy, and [].concat() is a native method that doesn't understand ArrayProxies, and just sees it as a single object to be pushed onto the array.

[stefanpenner]

this isn't optimal, but i don't believe it to be a bug either.

[wagenet]

One solution is to call toArray on the ArrayProxy, e.g. [].pushObjects(Ember.ArrayProxy.create({ content: [ 1, 2, 3 ] }).toArray()). I'm not sure if we should do this internally or just expect the developer to do it.

@stefanpenner if you're pretty sure it's not a bug should we close this?

[stefanpenner]

I'm not sure if we should do this internally or just expect the developer to do it.

Ya this is why I suspect why it isn't a bug. I don't believe we should be doing this for developers, as without monkey patching all native array methods, we can't hope to safe guard them from all reasonable scenarios. Also, we hope to continue to minimize the usage of proxies going forward.

[wagenet]

I'm going to go ahead and close it. @bendemboski I hope this makes sense.

[bendemboski]

Well, I see two issues.

This is either a bug, or it's a documentation bug because the documentation says that pushObjects() adds all objects in the passed enumerable to the end of the array, but if the implementation is correct then it should say that it only accepts a native array, not an enumerable.

Also, the reason I ran into this is that addObjects() works fine when passed an ArrayProxy, and I converted an addObjects() to a pushObjects() and it stopped functioning correctly.

A quick survey of similar methods reveals that addObjects() is actually the outlier -- pushObjects(), unshiftObjects(), removeObjects() and setObjects() all only work when passed a native array, whereas addObjects() works fine with an ArrayProxy.

I would think we would want all these methods to be consistent -- especially addObjects() and pushObjects(). If we only want to support native arrays being passed to these methods rather than any enumerables, then the documentation should be updated.

So, not sure where this leaves us as far as this bug, but I think there are some issues that definitely should be addressed.

[rwjblue]

Agreed. Unless there is a very compelling reason, they should all accept the same arguments.

[stefanpenner]

compelling reason

maybe not compelling, but the reasons why I am slightly unmotivated to consider this an issue worth fixing atm. Obviously if someone has energy and time to provide a quality tested PR that doesn't demonstrate no issues, i am sure we will be happy to accept it.

• potential performance overhead of intercepting each of these method calls.
• extra internal complexity
• plans to move away from proxies
• plans to move from enumerable to something es6 iterable compatible

[rwjblue]

I'm totally fine with them all requiring native arrays. I'm just saying it should be consistent either way.

Due to SemVer compat, we would need to deprecate passing an ArrayProxy (instead of removing support completely).

[stefanpenner]

Due to SemVer compat, we would need to deprecate passing an ArrayProxy (instead of removing support completely).

ya we need to find a way out of this mess, my preference is out entirely. Rather then more scenario solving, but this is a somewhat loosely held opinion

[rwjblue]

@stefanpenner - Totally agreed.

@bendemboski - Would you be able to take a look at submitting a PR to deprecate using ArrayProxy with addObjects?

[bendemboski]

Sure.

[bendemboski]

PR created -- #9854