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

Fixed: options.open.{appName, callback} #230

Merged
merged 3 commits into from
Sep 8, 2018
Merged

Conversation

bitnot
Copy link
Contributor

@bitnot bitnot commented Mar 8, 2017

Fixes #229 by passing open:appName config property to opn and calling open:callback.

@bitnot
Copy link
Contributor Author

bitnot commented Mar 8, 2017

@vladikoff could you kindly take a look?

@intih
Copy link

intih commented Mar 17, 2017

Good stuff 👍 Would like to see this merged as open.appName not working is an issue for me.

A suggestion for this PR: is it worth updating the readme to point to opn instead of open?

Also, opn allows you to specify a string or array for options.app - array to pass arguments to the app so the documentation should probably reflect that 😄

@XhmikosR
Copy link
Member

XhmikosR commented Sep 6, 2018

This needs a rebase, no unrelated style changes and no unrelated version changes.

@bitnot bitnot force-pushed the master branch 2 times, most recently from ea213f8 to 78b4c20 Compare September 8, 2018 06:33
@bitnot
Copy link
Contributor Author

bitnot commented Sep 8, 2018

Thanks for looking into this @XhmikosR!
Not sure if I should update the version and change log, please advise.

@XhmikosR
Copy link
Member

XhmikosR commented Sep 8, 2018

@bitnot: no you shouldn't update any meta files, this is done when a new release is made.

@@ -82,6 +82,7 @@ This can be one of the following:
callback: function() {} // called when the app has opened
}
```
Note that in [v0.9.0](https://github.com/gruntjs/grunt-contrib-connect/releases/tag/v0.9.0) [open](https://www.npmjs.com/package/open) was replaced with [opn](https://www.npmjs.com/package/opn) but the configuration remained the same for backwards compatibility. `targed`, `appName` and `callback` are the only supported keys in the config object.
Copy link
Member

Choose a reason for hiding this comment

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

targed is this correct or it is a typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, thanks again! Fixed.

Passing `open.appName` config property to `opn` and calling `open.callback`.
@XhmikosR
Copy link
Member

XhmikosR commented Sep 8, 2018

@bitnot: can you skip README.md inclusion? We usually update it when we make the release.

@XhmikosR XhmikosR merged commit 19924f8 into gruntjs:master Sep 8, 2018
@bitnot
Copy link
Contributor Author

bitnot commented Sep 8, 2018

@XhmikosR
I was a bit concerned users may try passing opn-style config with app instead of appName and with unsupported options, so felt need to clarify in the ReadMe.
Thanks for merging!

@XhmikosR
Copy link
Member

XhmikosR commented Sep 8, 2018

Will be in v2.0.0 when that is made.

Maybe you could add a test?

@bitnot
Copy link
Contributor Author

bitnot commented Sep 8, 2018

I am not very good with node, but I will try to add a test.

Any release date in mind?

@XhmikosR
Copy link
Member

XhmikosR commented Sep 8, 2018

It doesn't depend on me otherwise I'd made the release already. I don't have access to this npm package, so when #252 is merged.

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.

None yet

3 participants