Support all applicable JS Array methods on List and Results

[alazier]

From @appden :

I'd like to modify the array and results implementations to properly inherit from the native JS Array so we can leverage all of its methods. To do this, we just need to implement getters (and setters in the case of Array) for indices and the "length" property. JavaScriptCore will handle everything if we just do that (I believe the spec enforces this behavior, but I'm not quite sure of the details). Its native class name should be "Array" as well since many libraries expect this to check if an object is an array.

[alazier]

This will not work for Results as they are immutable. We can have parity in all non-mutating apis though and can explore this for RealmArray.

[appden]

This should work for Results as well. Mutations would just throw an exception. You can freeze arrays in JS (by using Object.freeze) thus creating immutable arrays. The only requirements for adopting all array behaviors are implementing index and length getters and setters. The builtin methods use these to accomplish everything.

Methods like map will return a new plain JS array at the moment, which is a good thing. Once JavaScriptCore follows the ES6 spec, we'd need to add Symbol.species to continue to have plain JS arrays returned.

[alazier]

We could make this work for Results but I don't think that we should. It is misleading to have methods which will always throw exceptions and this will also pollute the docs as well. We started off using this route in the objc binding and had to change ti to not inherit from native types in the end.

Additionally we will want our array to behave differently than the builtin array in many cases - and some methods we would like to prevent users from calling completely. Yes we could throw exceptions in these cases but I think it would be more elegant to simply have a non-derived Array like class that simply shares the methods and signatures we wish to support along with any Realm specific behaviors/methods.

[alazier]

I think we should rename Realm.Array to Realm.List to sufficiently differentiate from the functionality in Array that we don't want to support.

[appden]

Would you mind if we went over what functionality we don't want to support from Array? Of course, by inheriting from Array, we could still add new methods (and even alter existing ones, but I'll explain below why I'm not a fan of that). I can definitely speak from experience that JS devs (myself included) hate array-like objects that aren't actual arrays. :smile:

By only supporting getters for indices and length, these non-mutating builtin methods will just work: join, slice, indexOf, lastIndexOf, forEach, every, some, filter, map, reduce, reduceRight, as well as the many new methods added by ES6 (and its polyfills like the one in Babel). Conversely, adding setters would enable the mutating methods to work, though we could keep using our own custom mutator methods.

If we went down the path of not inheriting from Array, we should at least steal all of its prototype methods and make sure they work on our arrays. It's a pretty common pattern anyways in JS to do something like Array.prototype.map.call(list, callback) because you might be dealing with a NodeList or Array.prototype.push.apply(list, objects) because it's the only way you can push another list of objects directly onto an array (in this case, it would be better to call list.push.apply(list, objects), but I've seen it used the other way many times). These kind of examples are what make me want to just inherit from Array so we can guarantee everything Just Works™.

I don't think it would be enough to just document these limitations because devs will be passing these "arrays" down into 3rd-party libraries and getting weird exceptions that takes them awhile to figure out the cause.

I see your point with Results, though it's not uncommon to have methods in JS that just throw, such as with frozen arrays. It's preferable IMHO to have an immutable array than some array-like object that acts like an array (e.g. arguments and NodeList) because these inevitably end up giving you some exception that you need to track down and fix with something like Array.prototype.map.call(...) rather than just list.map(...).

ES6 added some niceties so array-like objects are a little nicer to deal with (like Array.from and Symbol.iterator), but also now officially allows you to inherit from Array in JS (it's easier for us in an ES5 environment because we're writing native code), so inheriting from Array would definitely be skating to where the puck is going.

Unfortunately, until the next version of JavaScriptCore is out in about a year, Array.isArray and Array.prototype.concat won't work with our array-subclass objects, but they should work in Chrome, so we could easily monkeypatch these methods in JavaScriptCore contexts because we have access to the JS context before anything else is evaluated.

[alazier]

We don't want the default implementations for Array methods as they will results on Object accessor creation. For this reason we should rename to List to avoid any confusion.

Even with this approach we should support users calling apply on a Results or List object using. This way a user is explicitly creating accessors and getting an Array object back.

[appden]

I'm fully in favor of renaming to List if we end up not inheriting methods from Array. :+1:

[alazier]

Array -> List is done in #37

At this point I am warming up to the idea of inheriting from Array if we can ensure that we can disable/override methods we don't want to support or for which we need custom behavior. This would be more a matter of convenience rather than something we would advertise to our users.

[alazier]

@appden lease list exactly what you think we should be supporting.

[appden]

Two more nice-to-have mutating methods on List that we can steal directly from Array.prototype:

• reverse
• sort

Non-mutating methods that List and Results could easily support just by stealing from Array.prototype:

• concat
• join
• slice
• forEach
• every
• some
• filter
• find
• findIndex
• map
• reduce
• reduceRight
• entries
• keys
• values
• @@iterator

These non-mutating methods would need native implementations since references to Realm objects are not unique and therefore cannot be compared with == (or ===, see #19):

• includes
• indexOf
• lastIndexOf

[alazier]

If you steal Array prototypes for reverse and sort they will be incredibly slow. In the case of reverse we want to be operating on the underlying ListView directly so we don't have to create two layers of accessors (c++ and js). Our internal version of sort which can sort by a property or list of properties should be preferred over any method which needs to execute js for every list item (which the builtin sort does). We need to construct our apis so that users don't use the existing sort by defualt without even looking for our preferred version. If this means changing the api name or not supporting the version that takes a function then I think that is strongly worth considering.

There are similar concerns for many of the other other methods you mention, and although using the Array.prototype implementation may work it is not necessarily desirable. This is definitely the case for filter, find, and findIndex, and I'm pretty sure we will not want default behaviors for some of these others. We need to be sure that we don't make our users believe Lists and Results are Arrays because they are NOT.

In the worst cast we could have a toArray method on Results and Lists and then users could use any of the default implementation. This seems clunky and I think we can probably come up with something better, but this would still be preferable to using the default Array implemenations for all methods.

[appden]

I'm pretty sure we will not want default behaviors for some of these others

If we name any of our methods the same as any of those, they absolutely should have the same behavior as Array. Our API would be an incredibly frustrating source of bugs otherwise.

I feel much more strongly about adding the non-mutating methods I listed than adding reverse and sort to List. Those can come later with our own optimized implementations if you really prefer. These performance concerns feel a bit like premature optimization to me. We could move forward with using the implementations already provided to us by JavaScriptCore, and then optimize according to real-world benchmarks. I'm all for adding our own methods as well, just like we have sortByProperty on Results already.

The list of non-mutating methods that we can steal from Array.prototype really can't be optimized much by us creating our own implementations. Most of them need to call callbacks and are already very well optimized by JavaScriptCore and should behave exactly the same with our own collections.

[alazier]

We need to come up with different names then. We definitely will be supporting the core query engine for our filter/search/sort apis and those should be preferred to the default implementations.

You are very incorrect that these methods can't be optimized beyond the prototype impelementation. By using the default array implementations, we need to create both c++ and javascript accessors for every element, and then the js engine will operate on these top level accessors by calling methods and accessing properties all the way through the accessor chain. This will be much much slower than accessing the lowest level object (tableView or listView) where we can simply set indexes with no need for accessor creation, and where we can sort and filter using the optimized query engine.

Lastly it is much easier to add stuff later than to ship it and take it way, so I would suggest we see what our beta users are actually trying to do in their apps and to provide them this functionality (be it custom or using Array's implementation) rather than being trigger happy and being stuck with a bunch of stuff that half works that needs to be deprecated later.