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

Notification on webserver port binding #1467

Closed
segrey opened this issue Jun 24, 2015 · 12 comments
Closed

Notification on webserver port binding #1467

segrey opened this issue Jun 24, 2015 · 12 comments

Comments

@segrey
Copy link
Contributor

segrey commented Jun 24, 2015

Based on #614

WebStorm Karma integration listens to webserver port binding event (i.e. when this line printed https://github.com/karma-runner/karma/blob/v1.0.0-0/lib/server.js#L61).

Now the only way to accomplish this is to parse standard output of Karma server and mine data from lines like:

INFO [karma]: Karma v0.12.37 server started at http://localhost:9876/

Such an approach seems to be not very safe, e.g. it'll fail if logLevel: config.LOG_DISABLE is set. Well, actually, there is a workaround for this case: LOG_DISABLE is not allowed https://github.com/karma-runner/karma-intellij/blob/master/lib/intellij.conf.js#L50. But still it doesn't feel right.

It'd be nice if plugins could use some API to listen to webserver port binding event. Thanks!

@dignifiedquire
Copy link
Member

Which part of the api exactly are you referring to? The plugin api or the node api?

@segrey
Copy link
Contributor Author

segrey commented Jun 24, 2015

@dignifiedquire No particular preference for API.
I was thinking of something like this

globalEmitter.on('karma_server_port_bound', function(port) {
});

Didn't test, but looks like there is another way:

webServer.on('listening', function() {
  // "config.port" is bound
});

The only problem here is that it depends on karma/lib/server.js implementation, i.e. that "config.port" is increased by one after each unsuccessful binding attempt.

dignifiedquire added a commit to dignifiedquire/karma that referenced this issue 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

@segrey I've implemented a solution in #1485 for you, with that you can do

var Server = require('karma').Server
var server = new Server(config, done)

server.once('browsers_ready', function () {
  var alwaysSafePort = server.get('config').port 
})

@segrey
Copy link
Contributor Author

segrey commented Jul 18, 2015

@dignifiedquire Sorry for the late reply. Thanks for looking into this. Really appreciate it.
Unfortunately, browsers_ready event is not fired exactly after webserver port binding, there could be a delay until browsers are ready. Nevertheless, browser_ready is useful event for IDE: WebStorm guesses this very moment using 'browserConnected' events. Thanks a lot for introducing it.

About webserver port binding event: I'm thinking of using

webServer.on('listening', function() {
  var actuallyBoundPort = config.port;
});

What do you think about legitimating this way?
By legitimating I mean adding it to https://github.com/karma-runner/karma/blob/master/docs/dev/04-public-api.md.

@dignifiedquire
Copy link
Member

Sure sounds good, let's use 'webserver_listening' event on the server to propagate the event. are you up for a quick PR on it?

@segrey
Copy link
Contributor Author

segrey commented Jul 18, 2015

I don't think a special event like webserver_listening is really needed. IMO, listening to http server events is quite clear. Also, listening in this way is backward compatible up to karma 0.10 (https://github.com/karma-runner/karma/blob/v0.10.0/lib/server.js), so no code changes needed.

The only thing I'd like to have is legitimating this way to protect from breaking changes. If a breaking change is needed, then this way will be supported along with a new way for at least one version.

@dignifiedquire
Copy link
Member

It's not needed, but I want all events to be on the Server object, rather than exposing the individual components, as that is much larger surface area to support if I have to ensure that the webserver object is always exposed.

@segrey
Copy link
Contributor Author

segrey commented Jul 18, 2015

Makes sense. Will create a PR.

@maksimr
Copy link
Contributor

maksimr commented May 29, 2016

@segrey In latest version of karma we emit event 'listening' when webServer started, seems it should fix this issue and we can refactor this code.

I think we can close this issue as fixed, @segrey what are you think? :)

@maksimr maksimr self-assigned this May 29, 2016
@maksimr
Copy link
Contributor

maksimr commented May 29, 2016

Related issue #1874

@segrey
Copy link
Contributor Author

segrey commented May 30, 2016

Thanks, great! That's just what's needed. For backward compatibility with previous karma versions webServer.on('listening', ...) will be used.

@segrey segrey closed this as completed May 30, 2016
@maksimr
Copy link
Contributor

maksimr commented May 30, 2016

@segrey No problem :)

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

No branches or pull requests

3 participants