598912 - change "notifications" module to use EventEmitter event registration model

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: Implements changes

Comment 2

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

http://github.com/Gozala/jetpack-sdk/pull/5

Comment 3

[Drew Willcoxon :adw]

Wait, why are you changing `this` to be the data?  `this` should be the options object to which the data and onClick are attached.

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 477908 [details] [diff] [review]
Implements changes

The patch itself is not good, because it doesn't update the doc.

Comment 5

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

Comment on attachment 477908 [details] [diff] [review]
Implements changes

(In reply to comment #3)
> Wait, why are you changing `this` to be the data?  `this` should be the options
> object to which the data and onClick are attached.

Erm, right, sorry, I missed that.  Thus this still needs to |call| the callback to set `this` appropriately.

Comment 6

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

Maybe I'm missing something, but change of not using |call| was intentional since that's a way `EventEmitter` works.

Comment 7

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

(In reply to comment #4)
> Comment on attachment 477908 [details] [diff] [review]
> Implements changes
> 
> The patch itself is not good, because it doesn't update the doc.

There is actually nothing to change on the documentation side that's why I don't.

Comment 8

[Drew Willcoxon :adw]

How is changing the meaning of `this` inside onClick related to EventEmitter?  Why would `this` be anything other than the object on which I've defined onClick?

The documentation explicitly mentions what arguments onClick will be passed and this.data.

Comment 9

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

Sorry Drew it seems that I misunderstood what switching to `EventEmitter` model meant. My understanding was that we'll be sticking to original interface they way it is implemented in browser, nodejs, etc.. But it seems we're not and in that case no change required at all.

If you want to know why IMO binding this is wrong:
https://bugzilla.mozilla.org/show_bug.cgi?id=598983#c3

Now I see that the example in the docs indeed mentions this.data

Comment 10

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

Attachment: v2

Updating implementation according to the comments.

Comment 11

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

c823f5a6d14b

Comment 12

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

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".