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

RESOLVED FIXED in Firefox 56

Status

()

defect
P1
normal
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: mano, Assigned: mak)

Tracking

unspecified
mozilla56
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment)

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.
How about doing:

function TransactionsHistory() {}

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

TransactionsHistory.prototype.myFunction = function () {
  // ...
};
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.
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.
Bug 838540 is about the "proper" solution, but it doesn't seem like anything is happening over there.
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/
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)
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__.
Assignee

Updated

2 years ago
Priority: -- → P3
Assignee

Updated

2 years ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [fxsearch]
Comment hidden (mozreview-request)

Comment 10

2 years ago
mozreview-review
Comment on attachment 8882767 [details]
Bug 982099 - Properly extend Array in PlacesTransaction.jsm.

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

Nice!
Attachment #8882767 - Flags: review?(standard8) → review+

Comment 11

2 years ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/6563cbe90004
Properly extend Array in PlacesTransaction.jsm. r=standard8

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6563cbe90004
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.