Events: QUnit logging system uses EventEmitter #644

Closed
wants to merge 1 commit into
from

Conversation

5 participants
@JamesMGreene
Member

JamesMGreene commented Aug 30, 2014

Fixes #422.

Even though this is earmarked for v2.0, it is 100% backward compatible.

Critique away. 馃槈馃嵎

@JamesMGreene JamesMGreene added this to the v2.0 milestone Aug 30, 2014

@JamesMGreene JamesMGreene added api and removed api labels Aug 30, 2014

@JamesMGreene

This comment has been minimized.

Show comment
Hide comment
@JamesMGreene

JamesMGreene Aug 30, 2014

Member

Shoot, I apparently need to rebase this. Hold tight.

Member

JamesMGreene commented Aug 30, 2014

Shoot, I apparently need to rebase this. Hold tight.

@JamesMGreene

This comment has been minimized.

Show comment
Hide comment
@JamesMGreene

JamesMGreene Aug 30, 2014

Member

Rebased.

Member

JamesMGreene commented Aug 30, 2014

Rebased.

@JamesMGreene

This comment has been minimized.

Show comment
Hide comment
@JamesMGreene

This comment has been minimized.

Show comment
Hide comment
@JamesMGreene

JamesMGreene Sep 2, 2014

Member

Should we switch the event names to be ES3-valid identifiers like @Krinkle mentioned in #531? e.g. "run-start" 鈫 "runStart", etc.

Member

JamesMGreene commented Sep 2, 2014

Should we switch the event names to be ES3-valid identifiers like @Krinkle mentioned in #531? e.g. "run-start" 鈫 "runStart", etc.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Sep 2, 2014

Member

I think this needs to wait until we get some feedback from other frameworks on the shared reporter. I just commented on the discussion issue, hopefully we get some participation there.

Member

jzaefferer commented Sep 2, 2014

I think this needs to wait until we get some feedback from other frameworks on the shared reporter. I just commented on the discussion issue, hopefully we get some participation there.

src/core.js
+ }
+
+ // Initialize collection of this logging callback
+ if ( QUnit.objectType( config.callbacks[ type ] ) === "undefined" ) {

This comment has been minimized.

@scottgonzalez

scottgonzalez Sep 3, 2014

Contributor

Can't this just be if ( !config.callbacks[ type ] )?

@scottgonzalez

scottgonzalez Sep 3, 2014

Contributor

Can't this just be if ( !config.callbacks[ type ] )?

This comment has been minimized.

@JamesMGreene

JamesMGreene Sep 3, 2014

Member

Yes, it was just copied from the old code.

@JamesMGreene

JamesMGreene Sep 3, 2014

Member

Yes, it was just copied from the old code.

src/core.js
+ }
+
+ callbacks = config.callbacks[ type ];
+ i = inArray( listener, callbacks );

This comment has been minimized.

@scottgonzalez

scottgonzalez Sep 3, 2014

Contributor

Do you want to guard against undefined here?

@scottgonzalez

scottgonzalez Sep 3, 2014

Contributor

Do you want to guard against undefined here?

This comment has been minimized.

@JamesMGreene

JamesMGreene Sep 3, 2014

Member

Yes, we should.

@JamesMGreene

JamesMGreene Sep 3, 2014

Member

Yes, we should.

src/core.js
+ i = inArray( listener, callbacks );
+ while ( i > -1 ) {
+ callbacks.splice(i, 1);
+ i = inArray( listener, callbacks );

This comment has been minimized.

@scottgonzalez

scottgonzalez Sep 3, 2014

Contributor

There's already a guard to prevent duplicate listeners, so this is unnecessary.

@scottgonzalez

scottgonzalez Sep 3, 2014

Contributor

There's already a guard to prevent duplicate listeners, so this is unnecessary.

This comment has been minimized.

@JamesMGreene

JamesMGreene Sep 3, 2014

Member

True.

@JamesMGreene

This comment has been minimized.

Show comment
Hide comment
@JamesMGreene

JamesMGreene Sep 3, 2014

Member

Updated per @scottgonzalez's comments (+1 more guard for undefined config.callbacks[ type ] in emit).

Member

JamesMGreene commented Sep 3, 2014

Updated per @scottgonzalez's comments (+1 more guard for undefined config.callbacks[ type ] in emit).

@Krinkle

This comment has been minimized.

Show comment
Hide comment
@Krinkle

Krinkle Sep 3, 2014

Member
  • Let's not store these in QUnit.config. It's already getting crowded in there with conceptually different types of data. This is probably best kept in a plain local variable.
  • Are the logs test suite and events test suite asserting the same things? If not, what's the difference? If so, we should drop logs and just a few back-compat tests to the events test suite.
Member

Krinkle commented Sep 3, 2014

  • Let's not store these in QUnit.config. It's already getting crowded in there with conceptually different types of data. This is probably best kept in a plain local variable.
  • Are the logs test suite and events test suite asserting the same things? If not, what's the difference? If so, we should drop logs and just a few back-compat tests to the events test suite.
@JamesMGreene

This comment has been minimized.

Show comment
Hide comment
@JamesMGreene

JamesMGreene Sep 4, 2014

Member

Let's not store these in QUnit.config. It's already getting crowded in there with conceptually different types of data. This is probably best kept in a plain local variable.

Fine by me. I just repurposed what was already there for the old logging callback system.

Are the logs test suite and events test suite asserting the same things? If not, what's the difference? If so, we should drop logs and just a few back-compat tests to the events test suite.

That's the idea, yes. I think it's just easier to leave all of the logs tests in place for now and axe them completely in v2.x rather than waste time surgically slimming them down now.

Member

JamesMGreene commented Sep 4, 2014

Let's not store these in QUnit.config. It's already getting crowded in there with conceptually different types of data. This is probably best kept in a plain local variable.

Fine by me. I just repurposed what was already there for the old logging callback system.

Are the logs test suite and events test suite asserting the same things? If not, what's the difference? If so, we should drop logs and just a few back-compat tests to the events test suite.

That's the idea, yes. I think it's just easier to leave all of the logs tests in place for now and axe them completely in v2.x rather than waste time surgically slimming them down now.

@JamesMGreene

This comment has been minimized.

Show comment
Hide comment
@JamesMGreene

JamesMGreene Sep 5, 2014

Member

Eliminated QUnit.config.callbacks.

Member

JamesMGreene commented Sep 5, 2014

Eliminated QUnit.config.callbacks.

src/core.js
+ return QUnit;
+ },
+
+ off: function( type, listener ) {

This comment has been minimized.

@jzaefferer

jzaefferer Sep 5, 2014

Member

What's the usecase of removing event listeners?

@jzaefferer

jzaefferer Sep 5, 2014

Member

What's the usecase of removing event listeners?

This comment has been minimized.

@JamesMGreene

JamesMGreene Sep 5, 2014

Member
  1. Standard EventEmitter behavior/API.
  2. Thinking ahead to Reporters, e.g. if Reporters become an event-driven interface, then we would need to remove event listeners if the Reporters (especially the default reporter) can be removed/disabled.
@JamesMGreene

JamesMGreene Sep 5, 2014

Member
  1. Standard EventEmitter behavior/API.
  2. Thinking ahead to Reporters, e.g. if Reporters become an event-driven interface, then we would need to remove event listeners if the Reporters (especially the default reporter) can be removed/disabled.
src/core.js
@@ -198,7 +199,70 @@ QUnit = {
QUnit.start();
}, config.testTimeout );
}
+ },
+
+ emit: function( type, data ) {

This comment has been minimized.

@jzaefferer

jzaefferer Sep 5, 2014

Member

What's the usecase for this method outside of QUnit?

@jzaefferer

jzaefferer Sep 5, 2014

Member

What's the usecase for this method outside of QUnit?

This comment has been minimized.

@JamesMGreene

JamesMGreene Sep 5, 2014

Member

Primarily as an extension point geared toward QUnit plugins, especially jquery/qunit-composite.

@JamesMGreene

JamesMGreene Sep 5, 2014

Member

Primarily as an extension point geared toward QUnit plugins, especially jquery/qunit-composite.

src/core.js
+ }
+ }
+
+ return QUnit;

This comment has been minimized.

@jzaefferer

jzaefferer Sep 5, 2014

Member

I suppose this allows chaining, but I don't see why that's needed. Its certainly not tested anywhere in this PR.

@jzaefferer

jzaefferer Sep 5, 2014

Member

I suppose this allows chaining, but I don't see why that's needed. Its certainly not tested anywhere in this PR.

This comment has been minimized.

@JamesMGreene

JamesMGreene Sep 5, 2014

Member

Correct, it's just to promote chaining. It's definitely not needed, nor tested but it is a pretty common aspect of EventEmitters. I would be happy to either (a) test it or (b) remove it.

@JamesMGreene

JamesMGreene Sep 5, 2014

Member

Correct, it's just to promote chaining. It's definitely not needed, nor tested but it is a pretty common aspect of EventEmitters. I would be happy to either (a) test it or (b) remove it.

This comment has been minimized.

@jzaefferer

jzaefferer Sep 5, 2014

Member

I'd rather drop this now and add it back when there is a good practical reason to include it.

@jzaefferer

jzaefferer Sep 5, 2014

Member

I'd rather drop this now and add it back when there is a good practical reason to include it.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Sep 5, 2014

Member

Exporting QUnit.on with the given event names looks good, though I hope we can delay landing this until there seems to be some consensus on the js-reporter side. Wouldn't want to release a new set of events that we then have to change again.

Otherwise, see my inline comments.

Member

jzaefferer commented Sep 5, 2014

Exporting QUnit.on with the given event names looks good, though I hope we can delay landing this until there seems to be some consensus on the js-reporter side. Wouldn't want to release a new set of events that we then have to change again.

Otherwise, see my inline comments.

@JamesMGreene

This comment has been minimized.

Show comment
Hide comment
@JamesMGreene

JamesMGreene Sep 5, 2014

Member

Exporting QUnit.on with the given event names looks good, though I hope we can delay landing this until there seems to be some consensus on the js-reporter side. Wouldn't want to release a new set of events that we then have to change again.

Agreed... hopefully. 馃檹

Member

JamesMGreene commented Sep 5, 2014

Exporting QUnit.on with the given event names looks good, though I hope we can delay landing this until there seems to be some consensus on the js-reporter side. Wouldn't want to release a new set of events that we then have to change again.

Agreed... hopefully. 馃檹

@JamesMGreene

This comment has been minimized.

Show comment
Hide comment
@JamesMGreene

JamesMGreene Sep 5, 2014

Member

I should also add a test to verify the prevention of duplicate listeners.

Member

JamesMGreene commented Sep 5, 2014

I should also add a test to verify the prevention of duplicate listeners.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Sep 5, 2014

Member

I'm be much more comfortable landing this with just QUnit.on as a new public method, without "chaining". We should investigate other approaches for qunit-composite and for both emit and off I'd rather see the actual need for that before exposing those methods. At least emit is used internally, so it should be trivial to expose that later.

Member

jzaefferer commented Sep 5, 2014

I'm be much more comfortable landing this with just QUnit.on as a new public method, without "chaining". We should investigate other approaches for qunit-composite and for both emit and off I'd rather see the actual need for that before exposing those methods. At least emit is used internally, so it should be trivial to expose that later.

@JamesMGreene

This comment has been minimized.

Show comment
Hide comment
@JamesMGreene

JamesMGreene Sep 5, 2014

Member

Can do.

We will definitely need off [and emit] in the future if the standard Reporter interface ends up being an EventEmitter, which is highly likely. However, they still do not necessarily need to be on the public API, just exposed to whatever mechanism is responsible for hooking up Reporters, e.g.

QUnit.addReporter = function( reporter ) {
  var emitter = {
    on: QUnit.on,
    emit: emit,
    off: off
  };
  reporter.register( emitter );
};
Member

JamesMGreene commented Sep 5, 2014

Can do.

We will definitely need off [and emit] in the future if the standard Reporter interface ends up being an EventEmitter, which is highly likely. However, they still do not necessarily need to be on the public API, just exposed to whatever mechanism is responsible for hooking up Reporters, e.g.

QUnit.addReporter = function( reporter ) {
  var emitter = {
    on: QUnit.on,
    emit: emit,
    off: off
  };
  reporter.register( emitter );
};
@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Sep 5, 2014

Member

That sounds good to me. Either way, we can add that later without hassle, and will have much less docs (=XML!) to write to release this. And if we're wrong about a lot of this stuff, there's less mess to fix.

Member

jzaefferer commented Sep 5, 2014

That sounds good to me. Either way, we can add that later without hassle, and will have much less docs (=XML!) to write to release this. And if we're wrong about a lot of this stuff, there's less mess to fix.

@JamesMGreene

This comment has been minimized.

Show comment
Hide comment
@JamesMGreene

JamesMGreene Sep 10, 2014

Member

Updates:

  • Eliminated chainability of QUnit.on.
  • Removed QUnit.off completely.
  • Hid QUnit.emit from the public API.
  • Renamed all of the events to match @jzaefferer's suggested event names from js-reporters/js-reporters#1.
  • Added test for duplicate listeners.
Member

JamesMGreene commented Sep 10, 2014

Updates:

  • Eliminated chainability of QUnit.on.
  • Removed QUnit.off completely.
  • Hid QUnit.emit from the public API.
  • Renamed all of the events to match @jzaefferer's suggested event names from js-reporters/js-reporters#1.
  • Added test for duplicate listeners.
@JamesMGreene

This comment has been minimized.

Show comment
Hide comment
@JamesMGreene

JamesMGreene Sep 11, 2014

Member

Rebased from latest master (including async-done, Promise support, and QUnit.skip).

Member

JamesMGreene commented Sep 11, 2014

Rebased from latest master (including async-done, Promise support, and QUnit.skip).

src/core.js
});
QUnit.load = function() {
config.pageLoaded = true;
+ emit( "runStart", {

This comment has been minimized.

@jzaefferer

jzaefferer Sep 11, 2014

Member

Shouldn't runStart replace begin? This looks like it was missed when rebasing.

@jzaefferer

jzaefferer Sep 11, 2014

Member

Shouldn't runStart replace begin? This looks like it was missed when rebasing.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Sep 11, 2014

Member

Apart from the one issue above, this looks good. I still don't think we should land this until there's some more consensus on the js-reporter end. Changing these names along with a new method for binding the callbacks is fine, but once we release this, I don't want us changing those names again.

Member

jzaefferer commented Sep 11, 2014

Apart from the one issue above, this looks good. I still don't think we should land this until there's some more consensus on the js-reporter end. Changing these names along with a new method for binding the callbacks is fine, but once we release this, I don't want us changing those names again.

@jzaefferer jzaefferer modified the milestones: JS Reporter, v2.0 Sep 13, 2014

@JamesMGreene JamesMGreene referenced this pull request in js-reporters/js-reporters Sep 17, 2014

Open

Standard API #3

0 of 2 tasks complete
@JamesMGreene

This comment has been minimized.

Show comment
Hide comment
@JamesMGreene

JamesMGreene Sep 25, 2014

Member

FYI, my git history got very messed up while trying to rebase this branch so I unfortunately felt the need to squash it to clean things up.

Member

JamesMGreene commented Sep 25, 2014

FYI, my git history got very messed up while trying to rebase this branch so I unfortunately felt the need to squash it to clean things up.

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Feb 6, 2015

Member

I doesn't look we're gonna have progress fast enough on js-reporters to wait to land this one. I believe it's good to rebase and be ready to land this to advance on 2.0

Member

leobalter commented Feb 6, 2015

I doesn't look we're gonna have progress fast enough on js-reporters to wait to land this one. I believe it's good to rebase and be ready to land this to advance on 2.0

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Oct 7, 2015

Member

@JamesMGreene, do you want to work on this? If you're busy I'll continue the work on the top of your commit (keeping the authorship credits).

Member

leobalter commented Oct 7, 2015

@JamesMGreene, do you want to work on this? If you're busy I'll continue the work on the top of your commit (keeping the authorship credits).

leobalter added a commit to leobalter/qunit that referenced this pull request Oct 22, 2015

leobalter added a commit to leobalter/qunit that referenced this pull request Oct 22, 2015

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Oct 25, 2015

Member

I'm closing this as the discussion should be followed at #882

Member

leobalter commented Oct 25, 2015

I'm closing this as the discussion should be followed at #882

@leobalter leobalter closed this Oct 25, 2015

leobalter added a commit to leobalter/qunit that referenced this pull request Oct 25, 2015

leobalter added a commit to leobalter/qunit that referenced this pull request Oct 27, 2015

leobalter added a commit to leobalter/qunit that referenced this pull request Feb 1, 2016

flore77 added a commit to flore77/qunit that referenced this pull request Aug 2, 2016

flore77 added a commit to flore77/qunit that referenced this pull request Aug 2, 2016

flore77 added a commit to flore77/qunit that referenced this pull request Aug 2, 2016

flore77 added a commit to flore77/qunit that referenced this pull request Aug 9, 2016

flore77 added a commit to flore77/qunit that referenced this pull request Aug 10, 2016

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