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.

Sign up for GitHub

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

Jump to bottom

Allow --open option to specify the browser to use #825

Merged
merged 18 commits into from
Aug 30, 2017
Merged

Allow --open option to specify the browser to use #825

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
30c4fbd
Add optionally browser parameter to --open
frankwinter Mar 3, 2017
b5f064e
Fix code style issues
frankwinter Mar 3, 2017
5b7f8e0
Fix last code style issue
frankwinter Mar 3, 2017
15ec8d3
Merge remote-tracking branch 'upstream/master' into open-specific-bro…
frankwinter Mar 11, 2017
75ef717
Replace array spread
frankwinter Mar 14, 2017
d78c1ae
Merge remote-tracking branch 'upstream/master' into open-specific-bro…
frankwinter Mar 14, 2017
52ad1a2
Merge remote-tracking branch 'upstream/master' into open-specific-bro…
frankwinter Mar 15, 2017
f04bba3
Fix conflicts
frankwinter Jul 10, 2017
8b64374
Merge branch 'master' of https://github.com/webpack/webpack-dev-serve…
frankwinter Jul 10, 2017
4d65ef1
Fix whitespace issue
frankwinter Jul 10, 2017
2c32834
Add .idea to gitignore and remove files
frankwinter Jul 10, 2017
682d47e
Fix whitespaces
frankwinter Jul 10, 2017
a18ae9c
Ignore and remove package-lock.json
frankwinter Jul 11, 2017
d3a46ae
refactor argument composition
frankwinter Jul 11, 2017
9ca67c9
Merge branch 'master' into open-specific-browser
frankwinter Aug 27, 2017
cb31c90
Merge branch 'master' into open-specific-browser
shellscape Aug 30, 2017
b847645
removing trailing spaces
shellscape Aug 30, 2017
a230754
improve open option description to match pr
shellscape Aug 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,5 @@ node_modules
/client/sockjs.bundle.js
/coverage
*.pem
.idea/
/package-lock.json
20 changes: 15 additions & 5 deletions bin/webpack-dev-server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ yargs.options({
describe: "close when stdin ends"
},
"open": {
type: "boolean",
type: "string",
describe: "Open default browser"
},
"useLocalIp": {
Expand Down Expand Up @@ -337,7 +337,11 @@ function processOptions(wpOpt) {
options.disableHostCheck = true;

if(argv["open"] || argv["open-page"]) {
options.open = true;
if(argv["open"] && argv["open"] !== "") {
Copy link
Contributor

Choose a reason for hiding this comment

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

you've changed the type for yargs to string on line 95, and an empty string is falsey - is there a reason you're checking the truthy value of args["open"] and also checking that it's not empty?

options.open = argv["open"];
} else {
options.open = true;
}
options.openPage = argv["open-page"] || "";
}

Expand Down Expand Up @@ -455,9 +459,15 @@ function reportReadiness(uri, options) {
if(options.historyApiFallback)
console.log(`404s will fallback to ${colorInfo(useColor, options.historyApiFallback.index || "/index.html")}`);
if(options.open) {
open(uri + options.openPage).catch(function() {
console.log("Unable to open browser. If you are running in a headless environment, please do not use the open flag.");
});
if(argv["open"] && argv["open"] !== "") {
Copy link
Contributor

Choose a reason for hiding this comment

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

you're already performing this check on line 340, and on 461 you're checking the truthy value of options.open which is set on 341 and 343. shouldn't this be if(typeof options.open === "string")?

open(uri + options.openPage, { app: options.open }).catch(function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

not a fan of two nearly identical calls to open and subsequent logging. should be something like this:

(this is pseudo code, don't copy and paste it)

var openOptions = {};
var openMessage = "Unable to open browser";

if (typeof options.open === "string") {
  openOptions = { app: options.open };
  openMessage =+ " :  " + options.open;
}

open(uri + options.openPage, openOptions).catch(function () {
  console.log(openMessage + "If you are running in a headless environment, please do not use the open flag.");
});

console.log(`Unable to open browser '${options.open}'. If you are running in a headless environment, please do not use the open flag.`);
});
} else {
open(uri + options.openPage).catch(function() {
console.log("Unable to open browser. If you are running in a headless environment, please do not use the open flag.");
});
}
}
if(options.bonjour)
console.log("Broadcasting \"http\" with subtype of \"webpack\" via ZeroConf DNS (Bonjour)");
Expand Down
11 changes: 9 additions & 2 deletions lib/optionsSchema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,8 +196,15 @@
"type": "boolean"
},
"open": {
"description": "Let the CLI open your browser.",
"type": "boolean"
"description": "Let the CLI open your browser with the URL.",
"anyOf": [
{
"type": "string"
},
{
"type": "boolean"
}
]
},
"useLocalIp": {
"description": "Let the browser open with your local IP.",
Expand Down