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

Replace app with options hash to allow disabling waiting for app close #14

Merged
merged 1 commit into from
Jun 29, 2015

Conversation

watson
Copy link
Contributor

@watson watson commented Jun 21, 2015

WARNING: This is a breaking change!

Fixes #2

It changes the api of the opn function from opn(target, [app], [callback]) to opn(target, [options], [callback]). The use of a custom app is still allowed through the new options.app property.

It introduces a new options.wait boolean (default: true) which can be set to false if you do not wish the spawned process to keep your program hanging if opn is called with a callback. The callback will in that case be called immediately when the process have finished spawning.

This PR have only implemented options.wait for the platforms darwin and win32. Am I correct in assuming that waiting isn't supported on other platforms?

@watson
Copy link
Contributor Author

watson commented Jun 21, 2015

@sindresorhus how do you normally ensure that the tests run? I guess there is no browser on Circle CI :(

@watson
Copy link
Contributor Author

watson commented Jun 26, 2015

@sindresorhus let me know if I can do anything to fix the tests, or if this should just be merged as is?

@sindresorhus
Copy link
Owner

Sorry about the delay. Been traveling. I just enabled it on Travis to see if I would be able to set up automated testing, but got busy. Just ignore it. Will review and merge today or tomorrow.

@watson
Copy link
Contributor Author

watson commented Jun 26, 2015

@sindresorhus no problem - and don't worry... I'm traveling my self, so I know how it is ;)


Defaults to `true` and means that opn will wait calling the callback (if one is given) until the app have been closed again. If set to `false` the callback will be called immediately after the child process have spawned.

Note that waiting is only possible if the app name is either `darwin` or `win32`.
Copy link
Owner

Choose a reason for hiding this comment

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

What does this mean? Did you mean that it will only wait on OS X and Windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because of this set of if-sentences in the main function:
https://github.com/sindresorhus/opn/blob/a20ea1810c2805b3126134042a8ca9841284aa48/index.js#L24-L65

It seems that the support to actually wait is only available for either darwin or win32 platforms - at least it only seem to be implemented for those two

Copy link
Owner

Choose a reason for hiding this comment

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

I was confused by app name. I guess you meant OS name.

But no, it will wait on Linux too, it's just default, so there's no flag.

@watson
Copy link
Contributor Author

watson commented Jun 29, 2015

@sindresorhus I've updated the PR with your suggestions

sindresorhus added a commit that referenced this pull request Jun 29, 2015
Replace app with options hash to allow disabling waiting for app close
@sindresorhus sindresorhus merged commit 85551c0 into sindresorhus:master Jun 29, 2015
@sindresorhus
Copy link
Owner

Thanks :)

@watson watson deleted the no-wait branch June 29, 2015 17:36
sindresorhus added a commit that referenced this pull request Jun 29, 2015
watson added a commit to watson/opn that referenced this pull request Jun 29, 2015
@shinnn shinnn mentioned this pull request Jun 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants