Implemented browser and browserPrivate #294
Conversation
|
Thanks for working on this. I would prefer to use https://github.com/sindresorhus/default-browser You also need to update the readme. |
index.js
Outdated
| return baseOpen({ | ||
| ...options, | ||
| app: { | ||
| name: open.apps[browserName], |
There was a problem hiding this comment.
You will need to do some normalisation of the names. browserName will be different depending on the OS.
index.js
Outdated
| open.apps = apps; | ||
| open.openApp = openApp; | ||
|
|
||
| module.exports = open; | ||
| export {open}; |
There was a problem hiding this comment.
| export {open}; | |
| export default open; |
package.json
Outdated
| @@ -50,7 +51,9 @@ | |||
| "dependencies": { | |||
| "define-lazy-prop": "^2.0.0", | |||
| "is-docker": "^2.1.1", | |||
| "is-wsl": "^2.2.0" | |||
| "is-wsl": "^2.2.0", | |||
| "url": "^0.11.0", | |||
| defineLazyProperty(apps, 'browser', () => 'browser'); | ||
|
|
||
| defineLazyProperty(apps, 'browserPrivate', () => 'browserPrivate'); | ||
|
|
||
| open.apps = apps; | ||
| open.openApp = openApp; |
There was a problem hiding this comment.
These should be named exports.
There was a problem hiding this comment.
You mean something like this?
export {apps};
export default open;|
|
||
| #### Supported apps | ||
|
|
||
| - [`chrome`](https://www.google.com/chrome) - Web browser | ||
| - [`firefox`](https://www.mozilla.org/firefox) - Web browser | ||
| - [`edge`](https://www.microsoft.com/edge) - Web browser | ||
| - `browser` - Default web browser | ||
| - `browserPrivate` - Default web browser in incognito mode |
There was a problem hiding this comment.
This needs to document which browsers it supports.
index.js
Outdated
| const supportedBrowsers = Object.keys(flags); | ||
| for (const supportedBrowser of supportedBrowsers) { | ||
| if (browserName.includes(supportedBrowser)) { | ||
| if (app === 'browserPrivate') { | ||
| appArguments.push(flags[supportedBrowser]); | ||
| } | ||
|
|
||
| return baseOpen({ | ||
| ...options, | ||
| app: { | ||
| name: open.apps[supportedBrowser], | ||
| arguments: appArguments | ||
| } | ||
| }); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
I'm not too sure how to normalise the names, but is this good enough? It just checks if the browserName string contains "chrome", "firefox" or "edge" since these are the only browsers supported right now.
There was a problem hiding this comment.
That is not going to work. What if the default browser is Chrome Canary?
There was a problem hiding this comment.
Sorry for the lack of replies. I don't know what to do besides converting to lowercase and stripping whitespace. Do you have any advice for normalisation?
There was a problem hiding this comment.
A list of browsers with their mapping on the different operating systems. We only support 3 browsers for this, so should not be hard to make a list like that.
For example, .chrome is Google Chrome on macOS, google-chrome on Linux.
|
Friendly bump |
|
You still need to do this: https://github.com/sindresorhus/open/pull/294/files#r1093000444 |
|
Can you fix the merge conflict? |
|
Hi @sindresorhus, would this be ok? |
|
Thanks :) |
Fixes #266
Added
open.apps.browserandopen.apps.browserPrivate.open.apps.browseropens the default browser.open.apps.browserPrivateopens the default browser in incognito mode for chrome, firefox and edge.I tested this on Windows, but I'm not sure if it works on other platforms.