724625 - refine unload module to match existing conventions

Status: RESOLVED WONTFIX

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

Description

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

Unload module is just like event emitter or maybe promise ? But it has a different API signature.

Comment 1

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

unload module should have same signature as other modules:

Option 1.

require('unload').on('unload', function() { })

Option 2.

require('unload').then(function() {})

Option 3.

require('system-hooks').on('unload', function() {})

Option 3.

require('system-hooks').unload.then(function() {})

Comment 2

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

Also currently unload implements `ensure` function that is GC friendly hack to make sure we don't keep objects alive for the lifetime of add-on. Instead on add-on unload we should trigger notification via https://developer.mozilla.org/en/nsiobserverservice
and in unload module register observers via weak references to provide same semantics as ensure but without additional hacks.

Comment 3

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

Also here is a documentation explaining how nsIWeakReference may be implement in js.

Comment 4

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

https://developer.mozilla.org/en/Weak_reference

Comment 5

[Alexandre Poirot [:ochameau]]

I was told by Blake that current nsIWeakReference was very hacky and may have multiple failures. So we should ask gabor if we can safely use them for this purpose. Otherwise, it sounds like a good idea.

At first sight, I do not have strong preference between 4 options, but we should keep support for old API. We may deprecate it, but we have to keep it working for multiple releases before dropping it. Even if it was a "low level" API, this API is currently used by many addons and they won't have much benefit during this API change.

In anycase, do not hesitate to split this in multiple pieces. I can already see two pieces:
- rewrite ensure in order to use weakrefs
- use promise pattern for unload

Comment 6

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

> At first sight, I do not have strong preference between 4 options, but we
> should keep support for old API. We may deprecate it, but we have to keep it
> working for multiple releases before dropping it. Even if it was a "low
> level" API, this API is currently used by many addons and they won't have
> much benefit during this API change.

They may not see much benefit, but that should not stop us from iterating. That, being said I' totally agree we can't just remove these modules, we will create alternative and make unload use it with a deprecation warnings.

> In anycase, do not hesitate to split this in multiple pieces. I can already
> see two pieces:
> - rewrite ensure in order to use weakrefs
> - use promise pattern for unload


In fact I was hoping to have one function only probably more like unload and something that extends Base and takes care of unloading unless destroyed. I'm not sure about the details yet.

Comment 7

[Gabor Krizsanits (INACTIVE)]

(In reply to Alexandre Poirot (:ochameau) from comment #5)
> I was told by Blake that current nsIWeakReference was very hacky and may
> have multiple failures. So we should ask gabor if we can safely use them for
> this purpose. Otherwise, it sounds like a good idea.

This is one of the scenarios weak pointers are designed for, so it seems like a good idea to me to use them. I don't know much about the weak reference implementation so I have no clue how could they fail. I will have to study weak references soon anyway, so I might get back you on this one, but you should ask Blake about this hacky thing a bit more specifically, I wonder what he meant.

Comment 8

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #1)
> unload module should have same signature as other modules:
> 
> Option 1.
> 
> require('unload').on('unload', function() { })
> 
> Option 2.
> 
> require('unload').then(function() {})
> 
> Option 3.
> 
> require('system-hooks').on('unload', function() {})
> 
> Option 4.
> 
> require('system-hooks').unload.then(function() {})

There is also:

Option 5.

require('system-hooks').unload().then(() => {});

Irakli what do you want to see here?  can we just add a promise/emitter version when we need it?

Comment 9

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

I was merely unhappy by the fact that this API was unique from everything else. At this point I think we should just use Disposable instead as it provides far more detailed control of unload hooks.

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

so can we wontfix this?