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: add support for chromium browsers #5

Merged
merged 4 commits into from
Apr 6, 2020

Conversation

yannbf
Copy link
Contributor

@yannbf yannbf commented Apr 6, 2020

Hey @ExiaSR, thanks for this lib! It's quite convenient.
I noticed that at the moment it is a bit outdated so I am making this PR to make it up to date.
CRA has recently added support for chromium based browsers and it's great so why not have it here as well, right? 😄

This PR also updates open to 7.0.2, and adds node_modules to .gitignore as it was not there and I believe it should.

Let me know what you think! I have tested it against chrome and brave and it worked great.

package.json Outdated Show resolved Hide resolved
if (!value) {
// Default.
action = Actions.BROWSER;
} else if (value.toLowerCase() === 'none') {
action = Actions.NONE;
} else {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CRA also takes care in case it runs on node scripts as you can see here. I left all that part out because I imagined there was a reason you didn't have it. Wanted to know your point of view about it!

Copy link
Owner

Choose a reason for hiding this comment

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

CRA also takes care in case it runs on node scripts as you can see here. I left all that part out because I imagined there was a reason you didn't have it. Wanted to know your point of view about it!

Some background story, gatsbyjs/gatsby#6550

I guess my intention was the script option is not a use case in gatsby, so I ended up excluding it. Also, I wasn't expecting people besides gatsby who would actually use this library.

I don't mind adding the script option back to better-opn, it should be done in a separate PR tho.

@michaellzc michaellzc merged commit 009601f into michaellzc:master Apr 6, 2020
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