browserstack / browserstack-local-nodejs

NodeJS bindings for BrowserStack Local
https://www.browserstack.com
MIT License
71 stars 56 forks source link

added default callbacks if not specified to start / stop #35

Closed jamesaspence closed 7 years ago

jamesaspence commented 7 years ago

This PR adds a default callback of an empty function to both start/stop. In the examples for start/stop, you end up doing something like:

browserstack_local.start(args, function () {
    console.log('browserstack local running');
});

browserstack_local.stop(function () {
    console.log('browserstack local closed');
});

This led me to assume that the two callbacks were optional, since they're not used for anything vital in this case. However, when running my tests locally and not specifying a callback, I got an error due to the callback not being specified.

This will make it so that, should a dev decide not to wait for the start / stop (something which a lot of devs might do), they can instead just call browserstack_local.start(args) / browserstack_local.stop() and continue onwards.

vedharish commented 7 years ago

Discussed with team and reverted the merge,

We want the code to error out if callback is not defined as we don't want anyone to interpret this as a sync function. The local binary is started only when the callback is executed.

The most common use case around start is to wait till the binary is started. If you don't want to wait for the callback, then providing an empty callback in the code is an easy workaround.

jamesaspence commented 7 years ago

In that case, can we add a user-friendly error to that function that specifies a required function?