Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(server): expose server event lifecycle in public API #1482

Conversation

nikaspran
Copy link

Return the EventEmitter instance so that it's possible to into various lifecycle events. Add a browsers_ready event so that it's possible to know when the karma server has finished starting up.

Enables and partially resolves #1037

I am very interested in resolving #1037 (it's breaking our grunt and gulp builds), so if this is not quite the way you want it to be done, let me know and I'll do it differently! :)

Return the EventEmitter instance so that it's possible to hook
into various lifecycle events. Add a "browsers_ready" event so
that it's possible to know when the karma server has finished
starting up.

Enables and partially resolves karma-runner#1037
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@nikaspran
Copy link
Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@dignifiedquire dignifiedquire self-assigned this Jul 8, 2015
@dignifiedquire dignifiedquire added this to the 0.13 milestone Jul 8, 2015
@dignifiedquire
Copy link
Member

So in general I like the approach, though I would prefer not returning the globalEmitter from the start function. In my mind the code (not only yours) needs to be refactored in a way that the interacting api would look like

var karma = require('karma')

karma.on('browsers_ready', function (event) {
  // ...
})

karma.start()

but I'm not a 100% sure yet what the best way is to introduce these changes into the code base.

@nikaspran
Copy link
Author

Thank you for taking a look at this :)

It seems quite a major undertaking to refactor everything but I agree it's the best idea longterm. I could take a stab if it's not in the near future for any of the regular maintainers.

One option to have this quickly and then still support easy migration/backwards compatibility later is to return a subset of the emitter, sort of an interface object. So something like this:

{on: globalEmitter.on.bind(globalEmitter), ...}

Then, in the future, it could just start returning karma itself. I think there's a general need to expand the public API a little bit to enable more diverse workflows.

dignifiedquire added a commit to dignifiedquire/karma that referenced this pull request Jul 8, 2015
This adds a slew of new api possibilities to the server. All main
events
from the `globalEmitter` are now emitted on the `server` instances and
publicly available.
For a list of available events see the docs file.

BREAKING CHANGE:

The public api interface has changed to a constructor form. To upgrade
change

```javascript
var server = require(‘karma’).server
server.start(config, done)
```

to

```javascript
var Server = require(‘karma’).Server
var server = new Server(config, done)
server.start()
```

Closes karma-runner#1037, karma-runner#1482, karma-runner#1467
@dignifiedquire
Copy link
Member

So it turned out, it wasn't that hard to do :) see #1485 and feel free to give it a test drive

@nikaspran
Copy link
Author

Oh, that's actually impressively quick! It seems like it will do the trick quite well. I think we're good to close this PR and the issue I mentioned if #1485 goes through. I'll play around with your branch tomorrow.

👍

@dignifiedquire
Copy link
Member

Great

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Public API does not provide hooks for server started, all browsers captured, etc
3 participants