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

Implemented browser and browserPrivate #294

Merged
merged 24 commits into from Mar 20, 2023
Merged

Implemented browser and browserPrivate #294

merged 24 commits into from Mar 20, 2023

Conversation

leslieyip02
Copy link
Contributor

Fixes #266

Added open.apps.browser and open.apps.browserPrivate.

open.apps.browser opens the default browser.
open.apps.browserPrivate opens 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.

@sindresorhus
Copy link
Owner

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],
Copy link
Owner

Choose a reason for hiding this comment

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

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};
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
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",
Copy link
Owner

Choose a reason for hiding this comment

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

url is a built-in module.

defineLazyProperty(apps, 'browser', () => 'browser');

defineLazyProperty(apps, 'browserPrivate', () => 'browserPrivate');

open.apps = apps;
open.openApp = openApp;
Copy link
Owner

Choose a reason for hiding this comment

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

These should be named exports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean something like this?

export {apps};
export default open;

Copy link
Owner

Choose a reason for hiding this comment

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

Yes


#### 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
Copy link
Owner

Choose a reason for hiding this comment

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

This needs to document which browsers it supports.

index.js Outdated
Comment on lines 115 to 130
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
}
});
}
});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

That is not going to work. What if the default browser is Chrome Canary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Owner

Choose a reason for hiding this comment

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

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.

@sindresorhus
Copy link
Owner

Friendly bump

@sindresorhus
Copy link
Owner

You still need to do this: https://github.com/sindresorhus/open/pull/294/files#r1093000444

@sindresorhus
Copy link
Owner

Can you fix the merge conflict?

@leslieyip02
Copy link
Contributor Author

Hi @sindresorhus, would this be ok?

@sindresorhus sindresorhus merged commit 3b79981 into sindresorhus:main Mar 20, 2023
2 checks passed
@sindresorhus
Copy link
Owner

Thanks :)

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.

To open url on default browser with incognito mode on cross platforms
4 participants