Skip to content

Events: QUnit logging system uses EventEmitter#644

Closed
JamesMGreene wants to merge 1 commit into
qunitjs:masterfrom
JamesMGreene:EventEmitter
Closed

Events: QUnit logging system uses EventEmitter#644
JamesMGreene wants to merge 1 commit into
qunitjs:masterfrom
JamesMGreene:EventEmitter

Conversation

@JamesMGreene

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member Author

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

@JamesMGreene

Copy link
Copy Markdown
Member Author

Rebased.

@JamesMGreene

Copy link
Copy Markdown
Member Author

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

@JamesMGreene

Copy link
Copy Markdown
Member Author

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

@jzaefferer

Copy link
Copy Markdown
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.

Comment thread src/core.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it was just copied from the old code.

@JamesMGreene

Copy link
Copy Markdown
Member Author

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

@Krinkle

Krinkle commented Sep 3, 2014

Copy link
Copy Markdown
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.

@JamesMGreene

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member Author

Eliminated QUnit.config.callbacks.

Comment thread src/core.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the usecase of removing event listeners?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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

Copy link
Copy Markdown
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.

@JamesMGreene

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member Author

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

@jzaefferer

Copy link
Copy Markdown
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.

@JamesMGreene

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
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.

@JamesMGreene

Copy link
Copy Markdown
Member Author

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#1.
  • Added test for duplicate listeners.

@JamesMGreene

Copy link
Copy Markdown
Member Author

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

Comment thread src/core.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jzaefferer

Copy link
Copy Markdown
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.

@jzaefferer jzaefferer modified the milestones: JS Reporter, v2.0 Sep 13, 2014
@JamesMGreene JamesMGreene mentioned this pull request Sep 17, 2014
2 tasks
@JamesMGreene

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
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

@leobalter

Copy link
Copy Markdown
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).

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

Copy link
Copy Markdown
Member

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

@leobalter leobalter closed this Oct 25, 2015
leobalter pushed a commit to leobalter/qunit that referenced this pull request Oct 25, 2015
leobalter pushed a commit to leobalter/qunit that referenced this pull request Oct 27, 2015
leobalter pushed a commit to leobalter/qunit that referenced this pull request Feb 1, 2016
flore77 pushed a commit to flore77/qunit that referenced this pull request Aug 2, 2016
flore77 pushed a commit to flore77/qunit that referenced this pull request Aug 2, 2016
flore77 pushed a commit to flore77/qunit that referenced this pull request Aug 2, 2016
flore77 pushed a commit to flore77/qunit that referenced this pull request Aug 9, 2016
flore77 pushed 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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Implement QUnit callbacks event listener style

5 participants