Refactor modules and classes to not be an EventEmitter #103

Closed
dglol opened this Issue Mar 19, 2012 · 19 comments

Comments

Projects
None yet
3 participants
Contributor

dglol commented Mar 19, 2012

We should centralize events just like we centralized the preferences. It doesn't make sense for every module to emit its own events. By centralizing events, I mean to have only two Event emitters: EventEmitter and self.

This will enhance maintainability and it will reduce the overhead of the unnecessary mixin. Also, when EventEmitter is deprecated, it will be easier to swap out EventEmitter.

Contributor

whimboo commented Mar 19, 2012

So you mean to have a base class for all the modules which have to send events? If not, do you have a bit of pseudo code to explain it in more detail?

Contributor

dglol commented Mar 19, 2012

So you mean to have a base class for all the modules which have to send events?

Yes, that is essentially it.

Contributor

whimboo commented Mar 19, 2012

Also when will the replacement of EventEmitter happen? Is there a timeline for it? Can you reference to a bug? Would be nice to know so we can see if work like this would be important to have.

Contributor

dglol commented Mar 19, 2012

Worth looking at:
mozilla/addon-sdk#312
mozilla/addon-sdk#355

It's already on master; I just don't know when it will land.

Contributor

dglol commented Mar 19, 2012

Discussed this with Alex. The most important take away was this:

so to be more concrete, both upcoming versions (1.6/1.7) will still have events.js. It might be sending deprecate warning in 1.7 if gozala insist in sending some

Also he mentioned that it is likely that EventEmitter will be removed in the near future (1.8 or so I'm guessing). However, the new events module will not be available until 1.7.

Contributor

whimboo commented Mar 19, 2012

If it's on master the new code will be available in 1.7. Here the actual merge into the code base:
mozilla/addon-sdk@6a7ddbf

Collaborator

xabolcs commented May 28, 2013

EventEmitter is deprecated since 1.13.

@dglol, would you have time to write a specification for this issue, allowing me to contribute?

Contributor

dglol commented May 28, 2013

Hi @xabolcs,

Originally, we built memchaser with what was available, and at the time the
best way to create objects was using EventEmitter as a base class. However,
we found out that it would soon be deprecated and we would need to rewrite
our modules with the new building blocks which appears to be event/core and
event/target. The task of this issue is to rewrite garbage-collector,
memory/reporter using the new event modules.

On Tue, May 28, 2013 at 4:38 PM, Szabolcs Hubai notifications@github.comwrote:

EventEmitter is deprecatedhttps://addons.mozilla.org/en-US/developers/docs/sdk/1.13/modules/sdk/deprecated/events.htmlsince 1.13.

@dglol https://github.com/dglol, would you have time to write a
specification for this issue, allowing me to contribute?


Reply to this email directly or view it on GitHubhttps://github.com/mozilla/memchaser/issues/103#issuecomment-18578361
.

Collaborator

xabolcs commented Feb 19, 2014

After upgradig communication from port object to postMessage the documents are more clearer. :)
I think, I could give it a try.

@dglol, to clear the objectives:

  • is the original description (centralize message emitters) still valid?
  • or it is enough to upgrade those modules to use event/core and event/target as you wrote in your mail?
Collaborator

xabolcs commented Sep 18, 2014

@whimboo could you assign me for this issue, and append it to my queue after the JPM issue?
Thanks!

Contributor

whimboo commented Sep 19, 2014

Lets assign issues when you actually get started with. That way we might be able to find others who would also have interest.

Collaborator

xabolcs commented May 27, 2015

EventEmitter is gone near Firefox 40, see Travis build 64179805!

Collaborator

xabolcs commented May 27, 2015

Bug 1158534 - End Traits ... landed as b1a9b43c430c.

Collaborator

xabolcs commented May 27, 2015

@whimboo this issue have to be fixed also for Firefox 40!

Please choose what to fix first! :)

Contributor

whimboo commented May 29, 2015

To be able to properly test the upcoming front-end changes we should first fix the back-end modules. So getting rid of emitters and using the new proposed technology sounds like the first step to do.

Collaborator

xabolcs commented May 29, 2015

Sure!

xabolcs self-assigned this May 29, 2015

@xabolcs xabolcs added a commit to xabolcs/memchaser that referenced this issue May 31, 2015

@xabolcs xabolcs Issue #103 - garbage-collector.js: hacked to event target 7c07344

@xabolcs xabolcs added a commit to xabolcs/memchaser that referenced this issue Jun 3, 2015

@xabolcs xabolcs Issue #103 - Rename observer, remove error listener 3ffceb5

@xabolcs xabolcs added a commit to xabolcs/memchaser that referenced this issue Jun 3, 2015

@xabolcs xabolcs Issue #103 - memory.js: hello event/core f4cd153
Collaborator

xabolcs commented Jun 4, 2015

@dglol, @whimboo
PR #217 is for the EventEmitter part.

The original description (centralize message emitters) is still valid?

Contributor

whimboo commented Jun 5, 2015

Not sure if it is still valid. Lets get the first part in and we can see. Btw. David is not with us since 2012.

xabolcs added this to the 0.7 milestone Jun 5, 2015

@xabolcs xabolcs added a commit to xabolcs/memchaser that referenced this issue Jun 5, 2015

@xabolcs xabolcs Issue #103 - Addressed comments 60e5c41

@xabolcs xabolcs added a commit to xabolcs/memchaser that referenced this issue Jun 6, 2015

@xabolcs xabolcs Issue #103 - CollectorReporter 860275b

@xabolcs xabolcs added a commit to xabolcs/memchaser that referenced this issue Jun 12, 2015

@xabolcs xabolcs Replace EventEmitter with core events (#103) dce52bb
Contributor

whimboo commented Jun 12, 2015

PR #217 has been merged! Hurray for a working memchaser addon again. Thanks @xabolcs.

whimboo closed this Jun 12, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment