Skip to content

Events: EventEmitter logging system#981

Closed
nikhilshagri wants to merge 7 commits into
qunitjs:masterfrom
nikhilshagri:EventEmitter
Closed

Events: EventEmitter logging system#981
nikhilshagri wants to merge 7 commits into
qunitjs:masterfrom
nikhilshagri:EventEmitter

Conversation

@nikhilshagri

Copy link
Copy Markdown

Builds upon the EventEmitter model PR ( #882 ) . Adds event details as specified in the js-reporters draft proposal. Two test files( test/event.js and test/logs.js ) have been modified so that all assertions within them pass.

@jquerybot

Copy link
Copy Markdown

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

📝 Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.


If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

Comment thread test/logs.js
name: module2Test3.name,
suiteName: module2Context.name,
testName: module2Test3.testName,
status: "skipped",

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.

this is a big issue for implementing the events emitter: we can't simply remove the other properties, that would represent a breaking change and would require a major update.

In our api development process, we want to give a reasonable time before any major update to allow users to migrate from the old to the new API, otherwise upgrading a major version would be very hard in many suites.

I need to check this EventEmiter api and maybe releasing it with the new properties coming on a follow-up PR might be easier. That's also gonna need to rebased after some changes happening right now at the current code.

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.

Since it looks like the new event emitter API won't make it into 2.0.0, can we release the new API somewhere in a 2.x release, while keeping the existing callbacks intact? We could then remove the old callbacks in 3.0.

New events with old event data (or both old + new data) seems like a bad idea, so I think it would be better to add the new API with new data, then eventually remove the old API....

@leobalter

Copy link
Copy Markdown
Member

@cynicaldevil I'll continue this patch on #882, to keep the discussion there. Also, as discussed above, the event details will probably come separately onto another PR.

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.

5 participants