982099 - PlacesTransaction.jsm: Subclass arrays and be nice at the same time

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[Asaf Romano (gone)]

In the new PlacesTransaction.jsm I'm "subclassing" the native js-array in order to implement the TransactionsHistory object: adding methods, getters etc. No matter how I used all those ES5/6 techniques for doing so, the result wasn't very readable (it looked more like meta-programming, and that, of course, applies to the Proxies trick as well). At last, I decided not to be nice so the code could be, and used the old __proto__ hack:

let TransactionsHistory = [];
TransactionsHistory.__proto__ = {
  __proto__: Array.prototype,
...
};

I recall there's some upcoming ES6 API for making this task less painful (maybe it was an alternative to Object.defineProperties that takes a JS object and clones it. I'm not sure).

So I'm filing this bug for replacing the __proto__ there when/if a better way is available.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

How about doing:

function TransactionsHistory() {}

TransactionsHistory.prototype = Object.create(Array.prototype);

TransactionsHistory.prototype.myFunction = function () {
  // ...
};

Comment 2

[Asaf Romano (gone)]

This doesn't work because you must use the native array constructor, and you cannot just call it on another object.

I think Object.assign (bug 937855) is the API I was thinking of.

Comment 3

[Asaf Romano (gone)]

and the problem with the assignment isn't with functions but with getters. Object.definePropert[y|ies] is one ugly API for day-to-day use. Moreover, I tend to prefer having my implementation "contained" in their object literals. I know it doesn't matter (they end being set the same way on the same object) and I know it's very JS-ish to do myObj.aFunc = ... myObj.b = ..., but I honestly don't like it. However, if defining getters worked that way, I'd do that here.

Comment 4

[Asaf Romano (gone)]

Bug 838540 is about the "proper" solution, but it doesn't seem like anything is happening over there.

Comment 5

[Asaf Romano (gone)]

And there's also bug 885788.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

I found a little more information about subclassing arrays that might be useful:

http://perfectionkills.com/how-ecmascript-5-still-does-not-allow-to-subclass-an-array/

Comment 7

[Asaf Romano (gone)]

The sandbox trick is clever, isn't it? (that is, in the context of privileged code we could use Componets.utils.Sandbox for adopting the iframe trick suggested there)

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

The iframe trick is clever but seems very hacky to me, not sure we would want to do that. BTW, bug 948227 has landed recently that warns about the perf impact of using __proto__.

Comment 9

[Marco Bonardo [:mak]]

Attachment: Bug 982099 - Properly extend Array in PlacesTransaction.jsm.

Review commit: https://reviewboard.mozilla.org/r/153874/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153874/

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8882767 [details]
Bug 982099 - Properly extend Array in PlacesTransaction.jsm.

https://reviewboard.mozilla.org/r/153874/#review159130

Nice!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6563cbe90004
Properly extend Array in PlacesTransaction.jsm. r=standard8

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6563cbe90004