Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Bug 704357 - Updated implemenatation of event emitter and related APIs using Base and namespaces. #312

Closed
wants to merge 72 commits into from

2 participants

@Gozala
Owner

https://bugzilla.mozilla.org/show_bug.cgi?id=704357

This is a work in progress clean-up patch.

This change depends on #342

This change is a first draft towards big cleanup work in SDK. This batch implements event APIs without use of traits other property hiding schemes. Instead it uses namespaces for private data. Also it's was far from trivial to modify only necessary parts, so there are few places were much more complex rewrite was required. In addition to this old event API is still preserved via EventEmitter object that uses new one under the hood and logs warning on usage. While most of the APIs still use this EventEmitter it would be much easier to remove that dependency in the next iteration, which will remove Trait compositions.

All test pass both on stable and nightly builds.

Gozala added some commits
@Gozala Gozala Merge branch 'bug/compose-702835' into bug/ns-704357 7475db2
@Gozala Gozala Adding two until functions. e09fc0e
@Gozala Gozala Implement low level API for working with events using `namespaces`. 2765926
@Gozala Gozala Implement higher level API for working with events via `EventTarget` …
…exemplar.
5cf0652
@Gozala Gozala Merge remote-tracking branch 'upstream/master' into cleanup/events-70…
…4357

Conflicts:
	packages/api-utils/lib/utils/function.js
2810bbd
@Gozala Gozala Use new license block. fe42044
@Gozala Gozala Use shorter namespace syntax. 97fde27
@Gozala Gozala Update event target to use new license block. 270df43
@Gozala Gozala Fix regressions introduced by refactoring. f8ed17f
@Gozala Gozala fix event core tests. bdfaff8
@Gozala Gozala fix event target tests 0b232f8
@Gozala Gozala Refactor event traits to use event/core under the hood. ec5db4c
@Gozala Gozala Update registry to use new events API. e4559ba
@Gozala Gozala Update windows/observer to use new events API. 74c5729
@Gozala Gozala Update tabs to use new events API. ec59f38
@Gozala Gozala Update page-mod to use new events API. cfd7b2d
@Gozala Gozala Rename Enqueued to defer to match underscore. d203e5e
@Gozala Gozala Update tab/observer to use new events API. 00ee44e
@Gozala Gozala Update keyboard/observer to use new events API. e403f54
@Gozala Gozala Update content/loader to use new events API. e4ea047
@Gozala Gozala Update simple-prefs to use new events API. 394c471
@Gozala Gozala Update windows to use new events API. 3a95083
@Gozala Gozala Update tabs/tab to use new events API. 6d351de
@Gozala Gozala Fix incorrect test case. d200281
@Gozala Gozala Rewrite private-browsing to use new event emitter API. 164084d
@Gozala Gozala Fix regression in passwords. 5744120
@Gozala Gozala Also implement internal _emit / _removeAllListeners to make migration…
… path easier.
2188d86
@Gozala Gozala Switch to defer from Enqueued. 6d8d517
@Gozala Gozala Update some of the deprecated APIs. 178620f
@Gozala Gozala Reduce deprecated APIs even further. daf8d2c
@Gozala Gozala Remove usage of all deprecated APIs. 25945c3
@Gozala Gozala remove no longer necessary AsyncEventEmitter. 2024582
@Gozala Gozala remove debug log. ff3dbe9
@Gozala Gozala Update tests for Enqueued to defer. 1b94e3b
@Gozala Gozala update request to use new event emitter API. 8d30ff9
@Gozala Gozala Update simple storage to get rid of traits and deprecated event emitt…
…er APIs.
c83d46c
@Gozala Gozala Also simplify tests. 57684dc
@Gozala Gozala Update selections API to get rid off traits deprecated event API. 9e0a7fa
@Gozala Gozala Add an API for counting registered listeners. cc25548
@Gozala Gozala Update page-mod to a new event API. b534087
@Gozala Gozala Update widget so it no longer relies on event emitters private API. f1692cd
@Gozala Gozala Update panel to make it compatible with latest changes in event API. 72098da
@Gozala Gozala Get rid of temporary tab hack. 6d81416
@Gozala Gozala Reduce usage of deprecated event APIs. 05520d4
@Gozala Gozala Update panel to stop using deprecated APIs. 304b230
@Gozala Gozala simplify function. 5a8f2da
@Gozala Gozala Implement compose utility function. 6c5c411
@Gozala Gozala Update tests for function utils and add tests for compose. 0faef17
@ochameau ochameau was assigned
@ochameau ochameau commented on the diff
packages/addon-kit/lib/private-browsing.js
((87 lines not shown))
+
+// Make sure listeners are cleaned up.
+unload(function() off(exports));
+
+Object.defineProperty(exports, "isActive", { get: function() model.active });
@ochameau Owner

You haven't implemented isActive setter, that was implemented before:
https://github.com/mozilla/addon-sdk/pull/312/files#L3L49

@Gozala Owner
Gozala added a note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ochameau ochameau commented on the diff
packages/addon-kit/lib/private-browsing.js
((56 lines not shown))
- set isActive(value) {
- if (pbService)
- // We toggle private browsing mode asynchronously in order to work around
- // bug 659629. Since private browsing transitions are asynchronous
- // anyway, this doesn't significantly change the behavior of the API.
- setTimeout(toggleMode, 0, value);
- },
- _isActive: false
-})()
+ // set up an observer for private browsing switches.
+ observers.add('private-browsing-transition-complete', function onChange() {
+ // Update model state.
+ model.active = pbService.privateBrowsingEnabled;
+ // Emit event with in next turn of event loop.
+ deferredEmit(exports, model.active ? 'start' : 'stop');
+ });
@ochameau Owner

It looks like we are leaking onChange as we never release this observer on addon unload.
Either fix it or feel free to open a followup bug, as we were already leaking it in previous code.

@Gozala Owner
Gozala added a note

I don't think it's actually true as observer service uses a weak references for listeners, so they can be gc-ed without explicit un-registration.

@ochameau Owner

Tha's true, verified. On top of that, we keep a list of these observers that we unregister on unload.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
packages/addon-kit/lib/private-browsing.js
((87 lines not shown))
+
+// Make sure listeners are cleaned up.
+unload(function() off(exports));
+
+Object.defineProperty(exports, "isActive", { get: function() model.active });
+exports.activate = function activate() pbService && setMode(true)
+exports.deactivate = function deactivate() pbService && setMode(false)
+exports.on = on.bind(null, exports);
+exports.once = once.bind(null, exports);
+exports.removeListener = function removeListener(type, listener) {
+ off(exports, type, listener);
+};
@ochameau Owner

Can't we use bind as 2 previous lines?

@Gozala Owner
Gozala added a note

no as we do need to make sure listener is passed. Otherwise it will remove all listeners for the given event type.

@ochameau Owner

ok, may you add a comment? you have to perfectly know how off works in order to understand why removeListener is special.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
packages/addon-kit/lib/private-browsing.js
((87 lines not shown))
+
+// Make sure listeners are cleaned up.
+unload(function() off(exports));
+
+Object.defineProperty(exports, "isActive", { get: function() model.active });
+exports.activate = function activate() pbService && setMode(true)
+exports.deactivate = function deactivate() pbService && setMode(false)
+exports.on = on.bind(null, exports);
+exports.once = once.bind(null, exports);
+exports.removeListener = function removeListener(type, listener) {
+ off(exports, type, listener);
+};
+
+// Workaround to make sure weak map pointer is set before exports are frozen.
+off(exports, 'whatever');
@ochameau Owner

Can you add (feel free to pick one / many / all):

  • a link to the related code in Namespace module
  • a reference to bug 720666
  • a comment about this workaround in this bug, or the platform bug that will prevent using weakmap workaround in namespace (bug 673468)
  • a note about what is done in events/core so that the weakmap is inited
@Gozala Owner
Gozala added a note

In other places I wrote a better comment:

https://github.com/mozilla/addon-sdk/pull/312/files#L5R408

would similar be sufficient ?

@ochameau Owner

yes!

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

I'm wondering if we should deprecate EventEmitter immediatly.
Wouldn't it be better to first land the new implementation and use it for some limited set of modules.
See how it goes for one release, and avoid having to modify all modules at once,
and then, when we removed all dependencies to events.js:EventEmitter (traits version), we add console.warn.

Here is a step by step plan:
1/ We land events/.js + events.js modification, but we do no do console.warn, so that we can still use EventEmitter (in particular _emit) everywhere. (we do not land anything else in this particular commit)
2/ We release with this change, we start communicating about EventEmitter deprecation and about the new events/
.js
3/ In a next release, when we no longer use EventEmitter, we deprecate it for real by adding console.warn.

In parallel, as soon as we landed 1/, we can land other parts of this pull request.
But it would be really usefull to split this work again and land one commit per high level module.
It sounds really important if we face a regression and we want to backout something without having to backout everything.
I don't think we should land in-between refactoring, like page-mod, panel, request, selection, ... (as we can still use _emit if we do not deprecate it immediatly though warnings)
Instead, we may land only a single refactoring per API that remove Traits usage and use new events.
So that context-menu, simple-storage and private-browsing are good candidates.

Does that make sense? Is that clear?

@Gozala
Owner

I'm wondering if we should deprecate EventEmitter immediately.

If we do update all our APIs I think we should. If we decide that change is not ready for a given release we can always back out that plan.

Wouldn't it be better to first land the new implementation and use it for some limited set of modules.
See how it goes for one release, and avoid having to modify all modules at once,
and then, when we removed all dependencies to events.js:EventEmitter (traits version), we add console.warn.

I would prefer to not hold it for a multiple releases as patch will rotten, it already did three times in fact. If we run into
issues that can't be fixed soon enough we can fall back to that plan. That was a strategy for loader changes and I think
it worked out well enough, even though we had issues.

Here is a step by step plan:
1/ We land events/.js + events.js modification, but we do no do console.warn, so that we can still use EventEmitter (in
particular _emit) everywhere. (we do not land anything else in this particular commit)
2/ We release with this change, we start communicating about EventEmitter deprecation and about the new events/.js
3/ In a next release, when we no longer use EventEmitter, we deprecate it for real by adding console.warn.

It's a good strategy but will add a lot of maintenance work which I don't think is worth it. That being said we could land these changes in separate commits / merges so that we can back out only parts that are necessary. But again, this would require
additional work which may be not worth it. With loader changes our strategy was to land as is, and in case of unresolvable problems back out and land smaller chunks, I would rather prefer same strategy here. Try to land if happen to have problems we back out. Then land back change described in your /1

In parallel, as soon as we landed 1/, we can land other parts of this pull request.
But it would be really usefull to split this work again and land one commit per high level module.
It sounds really important if we face a regression and we want to backout something without having to backout everything.
I don't think we should land in-between refactoring, like page-mod, panel, request, selection, ... (as we can still use _emit > if we do not deprecate it immediatly though warnings)
Instead, we may land only a single refactoring per API that remove Traits usage and use new events.
So that context-menu, simple-storage and private-browsing are good candidates.

In fact they are separate commits so in case of back out I'll be able to create a branches and cherry pick into them seperate chunks like event/core, changes to widget, context-menu, etc..

Does that make sense? Is that clear?

This all makes sense, it just I would rather avoid spending more time on steps that may not be necessary. We can do this little ceremony if that would be required. I hope this also makes sense.

@Gozala
Owner

BTW we still can use EventEmitter (in particular _emit) anywhere. It's just it will log warning to make sure that you're aware that you will have to update this eventually.

@ochameau ochameau commented on the diff
packages/api-utils/lib/event/core.js
@@ -0,0 +1,145 @@
+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim:set ts=2 sw=2 sts=2 et: */
@ochameau Owner

We have the same code indentation rules in all our JS codebase, can't we document that instead of adding such header in all files?

@Gozala Owner
Gozala added a note

No the issues is that if someone else editing this file who has default of 4 spaces in editor will have to remember and fix. This way editors auto configure self's in order to use 2 spaces for this file regardless of general settings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
packages/api-utils/lib/event/core.js
((22 lines not shown))
+};
+
+/**
+ * Registers an event `listener` that is called every time events of
+ * specified `type` is emitted on the given event `target`.
+ * @param {Object} target
+ * Event target object.
+ * @param {String} type
+ * The type of event.
+ * @param {Function} listener
+ * The listener function that processes the event.
+ */
+function on(target, type, listener) {
+ let listeners = observers(target, type);
+ if (typeof(listener) !== 'function')
+ throw new Error(BAD_LISTENER);
@ochameau Owner

nit: this test would make more sense in the begining of the method. i.e. before let listeners = observers(target, type);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
packages/api-utils/lib/event/core.js
((24 lines not shown))
+/**
+ * Registers an event `listener` that is called every time events of
+ * specified `type` is emitted on the given event `target`.
+ * @param {Object} target
+ * Event target object.
+ * @param {String} type
+ * The type of event.
+ * @param {Function} listener
+ * The listener function that processes the event.
+ */
+function on(target, type, listener) {
+ let listeners = observers(target, type);
+ if (typeof(listener) !== 'function')
+ throw new Error(BAD_LISTENER);
+
+ if (!~listeners.indexOf(listener))
@ochameau Owner

Please use an obvious pattern, we do not have such size constraints!
if (listeners.indexOf(listener) == -1)

@Gozala Owner
Gozala added a note

We discussed this and I don't think we agreed to switch from this well known pattern on the web. In a way it may be a misuse but so is double negation expressions instead of type casting !!foo => Boolean(foo). It's not about size constraints it's about convenience. Also note that we use this form in many other places in the code, if we decide to switch we will have to change code in all these places and in any case it will be a separate task.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
packages/api-utils/lib/event/core.js
((61 lines not shown))
+
+/**
+ * Execute each of the listeners in order with the supplied arguments.
+ * All the exceptions that are thrown by listeners during the emit
+ * are caught and can be handled by listeners of 'error' event. Thrown
+ * exceptions are passed as an argument to an 'error' event listener.
+ * If no 'error' listener is registered exception will be logged into an
+ * error console.
+ * @param {Object} target
+ * Event target object.
+ * @param {String} type
+ * The type of event.
+ * @params {Object|Number|String|Boolean}
+ * Arguments that will be passed to listeners.
+ */
+function emit(target, type, message) {
@ochameau Owner

The nth-arguments features isn't really explicit, you used a better pattern here:
https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/events.js#L139

 * @param {value} ...
 * More arguments to pass to listeners.
 */
 _emitOnObject: function _emitOnObject(targetObj, type, event /* , ... */) {
@Gozala Owner
Gozala added a note

ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ochameau ochameau commented on the diff
packages/api-utils/lib/event/core.js
((90 lines not shown))
+ let args = Array.slice(arguments, 2)
+ let listeners = observers(target, type).slice()
+ while (listeners.length) {
+ try {
+ yield listeners.shift().apply(target, args);
+ }
+ catch (error) {
+ // If exception is not thrown by a error listener and error listener is
+ // registered emit `error` event. Otherwise dump exception to the console.
+ if (type !== 'error' && observers(target, 'error').length)
+ emit(target, 'error', error);
+ else
+ console.exception(error);
+ }
+ }
+}
@ochameau Owner
  • Is there any particular reason in using yield? I'm not against it, if we aren't trying to use it everywhere but just for some cases. I imagine you made this for context menu, in order to be able to stop iteration when a listener returns something valid? I don't think we should ban some particular JS/Mozilla features, but be conscious of what we use and avoid using everywhere everytime everything. And Proxy like yield/iterator allow to address some particular issues.
  • Same question for this slice/while/shift pattern, is this really necessary to create a temporary array? Can't we just use for or foreach and iterate on the original array? Looks like you intend to avoid side effects regarding your tests/docs?
  • I'm not sure that we should bind this experimental method to emit which is not. We may expose it without willing it, if another API expose emit.
@Gozala Owner
Gozala added a note
  • Is there any particular reason in using yield? I'm not against it, if we aren't trying to use it everywhere but just for some cases. I imagine you made this for context menu, in order to be able to stop iteration when a listener returns something valid? I don't think we should ban some particular JS/Mozilla features, but be conscious of what we use and avoid using everywhere everytime everything. And Proxy like yield/iterator allow to address some particular issues.

Yes it's for context menu only & I hope we'll be able to get rid of it at some point. In fact comment on top of this function is pretty vocal about
the fact that it should be used only in exceptional cases.

  • Same question for this slice/while/shift pattern, is this really necessary to create a temporary array? Can't we just use for or foreach and iterate on the original array?
  1. You cant escape loop when using forEach without throwing exceptions.
  2. forEach will take a function and we need to yield from this function not from the one passed to forEach.

Looks like you intend to avoid side effects regarding your tests/docs?

Not sure I got the question right, but yes we intentionally do not trigger listeners added in the same dispatch and that was always a case.

  • I'm not sure that we should bind this experimental method to emit which is not. We may expose it without willing it, if another API expose emit.

I kept it this way as emit.lazy describes exactly what it does, and avoids some publicity by not
exporting right from the module. Also, we won't expose emit in high level modules anyway as
it requires this as first argument, so even if we decide to expose it it will be emit.bind(null, this) in
which case lazy property won't be there. That being said I'm fine exporting it right from the
module with as different function if you still think it's better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
packages/api-utils/lib/event/core.js
((116 lines not shown))
+ * The type of event.
+ * @param {Function} listener
+ * The listener function that processes the event.
+ */
+function off(target, type, listener) {
+ let length = arguments.length;
+ if (length === 3) {
+ let listeners = observers(target, type);
+ let index = listeners.indexOf(listener);
+ if (~index)
+ listeners.splice(index, 1);
+ }
+ else if (length === 2) {
+ observers(target, type).splice(0);
+ }
+ if (length === 1) {
@ochameau Owner

nit: missing else

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
packages/api-utils/lib/event/core.js
((74 lines not shown))
+ * Arguments that will be passed to listeners.
+ */
+function emit(target, type, message) {
+ for each (let item in emit.lazy.apply(emit.lazy, arguments))
+ item;
+}
+
+/**
+ * This is very experimental feature that you should not use unless absolutely
+ * need it. Also it may be removed at any point without any further notice.
+ *
+ * Creates lazy iterator of return values of listeners. You can think of it
+ * as lazy array of return values of listeners for the `emit` with the given
+ * arguments.
+ */
+emit.lazy = function lazy(target, type, message) {
@ochameau Owner

Same comment about nth-argument feature of this function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
packages/api-utils/lib/event/target.js
((2 lines not shown))
+/* vim:set ts=2 sw=2 sts=2 et: */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+'use strict';
+
+const { on, once, off } = require('./core');
+const { method } = require('../utils/function');
+const { Base } = require('../base');
+
+const EVENT_TYPE_PATTERN = /^on([A-Z]\S+$)/;
+
+/**
+ * `EventTarget` is an exemplar for creating an objects that can be used to
+ * add / remove event listeners on them.
@ochameau Owner

nit: It would be usefull to mention events/core and how you can emit event to these registered listeners. We may also consider md file is enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
packages/api-utils/lib/event/target.js
((13 lines not shown))
+const EVENT_TYPE_PATTERN = /^on([A-Z]\S+$)/;
+
+/**
+ * `EventTarget` is an exemplar for creating an objects that can be used to
+ * add / remove event listeners on them.
+ */
+const EventTarget = Base.extend({
+ /**
+ * Method initializes `this` event source. It goes through properties of a
+ * given `options` and registers listeners for the ones that look like an
+ * event listeners.
+ */
+ initialize: function initialize(options) {
+ // Go through each property and registers event listeners for those
+ // that have a name matching following pattern (`onEventType`).
+ Object.keys(options = options || {}).forEach(function onEach(key) {
@ochameau Owner

nit: would be easier to read if you move options = options || {} on a seperate line, before this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
packages/api-utils/lib/event/target.js
@@ -0,0 +1,70 @@
+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim:set ts=2 sw=2 sts=2 et: */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+'use strict';
+
+const { on, once, off } = require('./core');
+const { method } = require('../utils/function');
+const { Base } = require('../base');
+
+const EVENT_TYPE_PATTERN = /^on([A-Z]\S+$)/;
@ochameau Owner

\S is very permissive, don't we want something more restricted, like \w or [\w:] or ... ?

@Gozala Owner
Gozala added a note

I don't know I think \S fine, but I don't have problem restricting it further to \w either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
packages/api-utils/lib/event/target.js
((23 lines not shown))
+ * event listeners.
+ */
+ initialize: function initialize(options) {
+ // Go through each property and registers event listeners for those
+ // that have a name matching following pattern (`onEventType`).
+ Object.keys(options = options || {}).forEach(function onEach(key) {
+ let match = EVENT_TYPE_PATTERN.exec(key);
+ let type = match && match[1].toLowerCase();
+ let listener = options[key];
+
+ if (type && typeof(listener) === 'function')
+ this.on(type, listener);
+ }, this);
+ },
+ /**
+ * Registers an event `listener` that is called every time events of
@ochameau Owner

nit: unexpected leading space before Registers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ochameau ochameau commented on the diff
packages/api-utils/lib/event/target.js
((34 lines not shown))
+ this.on(type, listener);
+ }, this);
+ },
+ /**
+ * Registers an event `listener` that is called every time events of
+ * specified `type` are emitted.
+ * @param {String} type
+ * The type of event.
+ * @param {Function} listener
+ * The listener function that processes the event.
+ * @example
+ * worker.on('message', function (data) {
+ * console.log('data received: ' + data)
+ * })
+ */
+ on: method(on),
@ochameau Owner

What about using this in on, once & all ?
Instead of fighting against prototype it would be simplier for everyone to embrace it instead of working around it.
This method function is yet another thing to grok before figuring out how Jetpack code is working.
It is easy to read, really easy. There is no doubt about that, but when you want to understand what's going on, or if you want to debug it and you get a complex stacktrace, it becomes not that easy.

So here, instead of:
on: method(on)
we would have:
on: on

https://github.com/Gozala/addon-sdk/blob/975b90f51a02d8a0b079c36631d47ce3a428e9fd/packages/api-utils/lib/events.js#L28
instead of:
emit.apply(null, [ this._public ].concat(Array.slice(arguments)));
we whould have:
emit.apply(this._public, arguments);

https://github.com/Gozala/addon-sdk/blob/975b90f51a02d8a0b079c36631d47ce3a428e9fd/packages/addon-kit/lib/private-browsing.js#L50
instead of:
exports.on = on.bind(null, exports);
we would have:
exports.on = on.bind(exports);

Anywhere else, if we want to use these functions directly, we will have to use call.
I know it is a con, but pros are: no unecessary functions (no backtrace pollution, no performance issue [I was told that methods aren't inlined in current JS engine]) and one thing jetpack-specific less to know (method function).

@Gozala Owner
Gozala added a note

What about using this in on, once & all ?
Instead of fighting against prototype it would be simplier for everyone to embrace it instead of working around it.

Nobody fights prototype here, there are no prototypes in 'event/core'. It's unreasonable to require use of call on every
invoke of 'event/core' function which is way more common case.

This method function is yet another thing to grok before figuring out how Jetpack code is working.
It is easy to read, really easy. There is no doubt about that, but when you want to understand what's going on,
or if you want to debug it and you get a complex stacktrace, it becomes not that easy.

As you can see from the rest of the patch direct use of event/core functions is way more common, so seeing
emit.call everywhere will be more common and will trigger more confusing than this. After all how often do you
really expect users to look at this code ? Not to often IMO at least not as often as event/core.emit.

So here, instead of:
on: method(on)
we would have:
on: on

https://github.com/Gozala/addon-sdk/blob/975b90f51a02d8a0b079c36631d47ce3a428e9fd/packages/api-utils/lib/events.js#L28
i nstead of:
emit.apply(null, [ this._public ].concat(Array.slice(arguments)));
we whould have:
> emit.apply(this._public, arguments);

https://github.com/Gozala/addon-sdk/blob/975b90f51a02d8a0b079c36631d47ce3a428e9fd/packages/addon-kit/lib/private-browsing.js#L50
instead of:
exports.on = on.bind(null, exports);
we would have:
exports.on = on.bind(exports);

Anywhere else, if we want to use these functions directly, we will have to use call.

Anywhere else is much more common cases then those exceptional cases you listed here, which BTW will go away
once will fully complete cleanup and transition to Base. Neither API requiring use of .call makes any sense.

I know it is a con, but pros are: no unecessary functions (no backtrace pollution, no performance issue
[I was told that methods aren't inlined in current JS engine]) and one thing jetpack-specific less to know (method function).

In traits and cortex's etc.. we do wrapping and delegation of each method call anyway and it was hardly an issue. In that regard there will only improvements and if it ever will turn out to be a bottleneck we can talk about optimization strategies
there, unless of course js engine will implement inlineing.

@Gozala Owner
Gozala added a note

@ochameau please stop bringing this up over and over again, we talked about coding style at work week and have decide to git rid of patterns that
we all agreed upon and this one was not on that list. It really matter of taste and there is no fight against prototype or this or whatever. It's unreasonable and error prone to implement event/core API so that one will always have to invoke via .call form. Also we do optimize for common case, which is
emit(target, type, event) and not for an exceptional ones like stack traces which arguably users should not even have to deal with. Or event for EventTarget which is an only special use case. Finally we don't pioneer different style of APIs here on of the most popular frameworks on the web http://underscorejs.org/ does the same, but again not because it fights
anything because it makes more sense there.

Let's defer this particular (use of utility functions) style discussions until we're finished with a decided cleanups, otherwise it's frustrating for both of us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
packages/api-utils/lib/events.js
((170 lines not shown))
},
+ _removeAllListeners: function(type) {
+ console.warn(WARNING);
+ type ? off(this._public, type) : off(this._public);
@ochameau Owner

nit: you may use if/else statement as you do not intent to return a value from this one-liner

@Gozala Owner
Gozala added a note

ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ochameau ochameau commented on the diff
packages/api-utils/lib/events.js
((211 lines not shown))
return this;
}
-};
-exports.EventEmitter = require("./traits").Trait.compose(eventEmitter);
-exports.EventEmitterTrait = require('./light-traits').Trait(eventEmitter);
+});
@ochameau Owner

Wouldn't it be better to use a single implementation for both traits and use return this._public; || this instead?

@Gozala Owner
Gozala added a note

Wouldn't it be better to use a single implementation for both traits and use return this._public; || this instead?

Not for the readability, also if by some reason light-traits object will have _public it will misbehave. In fact there are few exception cases where this is the case. Otherwise sure would make more sense!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ochameau ochameau commented on the diff
packages/api-utils/tests/test-event-target.js
((148 lines not shown))
+ let drax = Error('Draax!!');
+
+ target.on('message', function() { throw boom; });
+
+ emit(target, 'message');
+ assert.ok(~String(exceptions[0]).indexOf('Boom!'),
+ 'unhandled exception is logged');
+
+ target.on('error', function() { throw drax; });
+ emit(target, 'message');
+ assert.ok(~String(exceptions[1]).indexOf('Draax!'),
+ 'error in error handler is logged');
+};
+
+require('test').run(exports);
+
@ochameau Owner

I'm wondering if we can factorise test-event-core.js and test-event-target.js, as they look 90% the same?

@Gozala Owner
Gozala added a note

I'm not sure it's worth it really. For example we could change event/core completely, but that should not affect EventTarget and it's tests, so I thought it was a better choice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ochameau ochameau commented on the diff
packages/api-utils/tests/test-registry.js
@@ -49,6 +49,7 @@ exports['test:remove'] = function(test) {
fixture.remove({});
let object = fixture.add({});
+ let isRemoveEmitted = false;
@ochameau Owner

nit: is this relevant?

@Gozala Owner
Gozala added a note

yes it's used in tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
packages/api-utils/docs/event/core.md
@@ -0,0 +1,44 @@
+Module provides core (low level) API for working with events in the SDK. This
+API is mainly for implementing higher level event APIs.
+
+Event `listener` may be registered on any (event `target`) object using
+provided `on` function:
+
+ var { on, once, off, emit } = require('api-utils/event/core');
+ var target = { name: 'target' };
+ on(target, 'message', function listener(event) {
+ console.log('hello ' + event);
+ });
+
+Event may of specific `type` be emitted on any event `target` object using
@ochameau Owner

This sentence is missing a word: "Event may of specific type ..."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ochameau ochameau commented on the diff
packages/api-utils/docs/event/core.md
((9 lines not shown))
+ on(target, 'message', function listener(event) {
+ console.log('hello ' + event);
+ });
+
+Event may of specific `type` be emitted on any event `target` object using
+`emit` function. This will call all registered `listener`s for the given `type`
+on the given event `target` in the same order they were registered.
+
+ emit(target, 'message', 'event');
+ // 'hello event'
+ emit(target, 'data', { type: 'data' }, 'second arg');
+
+Registered event listeners may be removed using `off` function:
+
+ off(target, 'message');
+ emit(target, 'message', 'bye');
@ochameau Owner

nit: you may be more explicit by adding stdout result after each emit call, here and in previous/next examples.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
packages/api-utils/docs/event/target.md
((34 lines not shown))
+ // Do the thing once ready!
+ });
+
+### Removing listeners ###
+
+`EventTarget` interface defines API for unregistering event listeners, via
+`removeListener` method:
+
+ target.removeListener('message', onMessage);
+
+### More details ###
+
+Listeners registered during the event propagation (by one of the listeners)
+won't be triggered until next emit of the matching type:
+
+ let { emit } = require('api-utils/event/target');
@ochameau Owner

There is a typo, you specified target instead of core.
let { emit } = require('api-utils/event/core');

I think it would be usefull to add a dedicated paragraph about emit:
"# How firing events on an EventTarget object? #"

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

I still think we should land this in multiple clear and distinct revisions.
I reviewed these files:
docs/event/core.md
docs/event/target.md
lib/event/core.js
lib/event/target.js
tests/test-event-core.js
tests/test-event-target.js

1/ It doesn't make sense to land these files merged with anything else from this PR. As it doesn't affect anything, it only adds this new library. There is some comments to be addressed but it can land soon.
2/ Then, I already reviewed lib/events.js, but I still think it should be part of another revision. We may have to backout this, we may break things with this. There is two things here: implement EventTarget using Base (2/), and refactoring all APIs using base (3/). (3 depends on 2.)
3/ As events.js is already reviewed, we should be able to get to this point soon too! This last part is landing high-level refactoring one by one. You already refactored: context-menu, simple-storage and private-browsing. But really, it doesn't make sense to land all these in a single merge!

Finally, I still think we should delay deprecation of _emit in traits version. Let me explain why: page-mod, panel, request, simple-prefs, widget, windows are in a middle state whereas we know we are going to refactor them to Base. This middle state ends up being hard to follow, it still uses Traits but doesn't use EventTarget. We will modify almost all high-level modules because of this, we may introduce regression, whereas we know we are going to refactor them soon. To be honest I'm scared about all these refactoring and I think we should do our best to minimize risks. And avoid modifying code until a planned big refactoring sounds like a good idea to me. So it basically means: do not send warning message today and wait before modifying any API until we do refactor them to Base. (You already did that for context-menu, private-browsing, selection and simple-storage.)
I can be convinced that modifying them first to new events can be an iterative step. But the kind of refactoring you are doing with the switch to Base are really disrupting, so that most of the code is going to change during this next step.

@erikvold erikvold referenced this pull request from a commit in erikvold/addon-sdk
@Gozala Gozala Factor out simple-prefs module changes from pr #312 2f246b8
@ochameau
Owner

Are we still going to use this pull request, or can we close it ?

@Gozala Gozala closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 8, 2011
  1. @Gozala
  2. @Gozala

    Adding two until functions.

    Gozala authored
Commits on Dec 10, 2011
  1. @Gozala
  2. @Gozala
Commits on Feb 8, 2012
  1. @Gozala

    Merge remote-tracking branch 'upstream/master' into cleanup/events-70…

    Gozala authored
    …4357
    
    Conflicts:
    	packages/api-utils/lib/utils/function.js
  2. @Gozala

    Use new license block.

    Gozala authored
  3. @Gozala

    Use shorter namespace syntax.

    Gozala authored
  4. @Gozala
  5. @Gozala
  6. @Gozala

    fix event core tests.

    Gozala authored
  7. @Gozala

    fix event target tests

    Gozala authored
  8. @Gozala
  9. @Gozala
  10. @Gozala
  11. @Gozala
  12. @Gozala
  13. @Gozala
  14. @Gozala
  15. @Gozala
  16. @Gozala
  17. @Gozala
  18. @Gozala
  19. @Gozala
  20. @Gozala

    Fix incorrect test case.

    Gozala authored
  21. @Gozala
  22. @Gozala

    Fix regression in passwords.

    Gozala authored
  23. @Gozala
  24. @Gozala

    Switch to defer from Enqueued.

    Gozala authored
  25. @Gozala
  26. @Gozala
  27. @Gozala
  28. @Gozala
  29. @Gozala

    remove debug log.

    Gozala authored
  30. @Gozala
  31. @Gozala
Commits on Feb 9, 2012
  1. @Gozala
  2. @Gozala

    Also simplify tests.

    Gozala authored
  3. @Gozala
  4. @Gozala
  5. @Gozala
  6. @Gozala
  7. @Gozala
  8. @Gozala

    Get rid of temporary tab hack.

    Gozala authored
  9. @Gozala
  10. @Gozala
  11. @Gozala

    simplify function.

    Gozala authored
  12. @Gozala
  13. @Gozala
  14. @Gozala

    Merge branch 'bug/functional-utils' into cleanup/events-704357

    Gozala authored
    Conflicts:
    	packages/api-utils/lib/utils/function.js
    	packages/api-utils/tests/test-function-utils.js
  15. @Gozala
  16. @Gozala
  17. @Gozala
  18. @Gozala
  19. @Gozala
  20. @Gozala
  21. @Gozala
  22. @Gozala
  23. @Gozala
Commits on Feb 14, 2012
  1. @Gozala
Commits on Mar 5, 2012
  1. @Gozala
  2. @Gozala
  3. @Gozala
  4. @Gozala

    Add more comments to make it more clear that `emit` and `emit.lazy` c…

    Gozala authored
    …an dispatche multiple arguments.
  5. @Gozala
  6. @Gozala
  7. @Gozala
  8. @Gozala
  9. @Gozala
  10. @Gozala
  11. @Gozala
  12. @Gozala
  13. @Gozala
Something went wrong with that request. Please try again.