Events: QUnit logging system uses EventEmitter

[JamesMGreene]

Fixes #422.

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

Critique away. 😉🍷

[JamesMGreene]

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

[JamesMGreene]

Rebased.

[JamesMGreene]

@jzaefferer @Krinkle @leobalter @scottgonzalez: Reviews, please?

[JamesMGreene]

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

[jzaefferer]

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.

[scottgonzalez]

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

[JamesMGreene]

Yes, it was just copied from the old code.

[JamesMGreene]

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

[Krinkle]

• 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]

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]

Eliminated QUnit.config.callbacks.

[jzaefferer]

What's the usecase of removing event listeners?

[JamesMGreene]

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.

[jzaefferer]

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]

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]

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

[jzaefferer]

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]

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]

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]

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 Standard Events/Hooks js-reporters/js-reporters#1.
• Added test for duplicate listeners.

[JamesMGreene]

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

[jzaefferer]

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

[jzaefferer]

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.

[JamesMGreene]

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]

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]

@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]

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