614259 - require("tabs") should be the object over which one can iterate tabs

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

The fix for Tabs API e10s compatibility (bug 598981) changed the API of the tabs module such that the collection of tabs over which one can iterate is exported as a "tabs" property of the object returned by require("tabs") rather than that object itself.

But the API was designed to enable consumers to access the collection of tabs as the return value of the require("tabs") call itself, not a property of it, and this change thus represents a regression in that API that we need to correct.

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

To my knowledge in jetpack we don't support a way of overriding exports, which makes it's pretty hard to satisfy this requirement. I also would like to justify
reason for this change:

1) Unfortunately I can't find a bug right now but there was a requirement to make tab collection a list. Main advantage that List has over collections that were used before is ability to get elements in array like manner:
var { tabs } = require('tabs');
var secondTab = tabs[1];

2) Windows API follows the same pattern as tabs do right now:
var { browserWindows } = require('windows');

Comment 2

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Good option to satisfy this requirement may be implementing another commonjs standard that we don't support yet:

module.exports = myCustomExports

Comment 3

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #1)
> To my knowledge in jetpack we don't support a way of overriding exports, which
> makes it's pretty hard to satisfy this requirement.

Although the exports object cannot be overridden, it can be dressed (f.e. a custom iterator can be added to it), and the most important consideration is what is the best API for the given set of use cases, not the relative difficulty of implementing that API.  And we know we can implement this API, because we have done so already.


> 1) Unfortunately I can't find a bug right now but there was a requirement to
> make tab collection a list. Main advantage that List has over collections that
> were used before is ability to get elements in array like manner:
> var { tabs } = require('tabs');
> var secondTab = tabs[1];

I don't recall establishing such a requirement from an API perspective, although it's possible that we discussed using List as an implementation detail.

I do see the value of being able to refer to tabs by index, although I'm also concerned about the implications of suggesting that the object is an array (and thus can be manipulated like an array), which raises a whole host of API questions.

In any case, this capability is not part of the public API, while iterating the object returned by the require statement is part of that API, and we should continue to support doing so.


> 2) Windows API follows the same pattern as tabs do right now:
> var { browserWindows } = require('windows');

require("windows") exports a browserWindows property in order to preserve our ability to allow iteration of all windows in the future via iteration of the object returned by require("windows").


(In reply to comment #2)
> Good option to satisfy this requirement may be implementing another commonjs
> standard that we don't support yet:
> 
> module.exports = myCustomExports

Sure.  But we don't need to do that in order to fix this regression.  We just need to fix the regression.

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Dietrich can you review the fix ?
https://github.com/mozilla/addon-sdk/pull/46

Comment 5

[Dietrich Ayala (:dietrich)]

Looks good, r=me.

Also, +1 on Myk's comment about the danger of array-seeming things. Can be terribly frustrating.

Comment 6

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I never suggested to present it like an array in fact doc's were suggesting that before I changed this.

I also do think that this is way more frustrating!

require('tabs')[1] // undefined
// So how should I get my tab ?
let i = 0, myTab;
for each (let tab in tabs) if (i == 1) myTab = tab;


Finally I do think that jetpack API's should not contradict to the js semantics:

for (let tab in tabs) 

should never be an object as it was a case.

Comment 7

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

1760c80