-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Allow --open option to specify the browser to use #825
Allow --open option to specify the browser to use #825
Conversation
Codecov Report
@@ Coverage Diff @@
## master #825 +/- ##
=======================================
Coverage 73.17% 73.17%
=======================================
Files 5 5
Lines 451 451
Branches 143 143
=======================================
Hits 330 330
Misses 121 121 Continue to review full report at Codecov.
|
bin/webpack-dev-server.js
Outdated
if(typeof options.open === "string") | ||
params.push({ app: options.open }); | ||
|
||
open(...params).catch(function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Array spread doesn't seem to work in Node v4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work because yargs still treats the open
command as a boolean. On line 89 you should change the type to string
.
Any chances to have it merged? :) |
@sergeymorkovkin we need tests added to this PR for the new option. it'll become too difficult to manually test for each new version otherwise. @frankwinter needs to add that and resolve the conflicts recently introduced for this to get merged I'm afraid. |
@shellscape Got it. Thanks for an update. If you think I could do this, please let me known. |
…into open-specific-browser
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll want to add .idea
to .gitignore and remove all of those files. You've got some whitespace issues as well on line 341
@@ -0,0 +1,12 @@ | |||
<component name="InspectionProjectProfileManager"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please blacklist and remove all the IDE specific files .idea/*
bin/webpack-dev-server.js
Outdated
|
||
if(argv["open"] || argv["open-page"]) { | ||
if(argv["open"] && argv["open"] !== "") { | ||
options.open = argv["open"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indendation seems to be off here
@frankwinter you'll have to remove the package-lock.json as well. |
Sorry, please excuse the clumsy workflow.. It's my first contribution on a forked github-project :-/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this still needs some help before we can merge it
bin/webpack-dev-server.js
Outdated
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"] !== "") { |
There was a problem hiding this comment.
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")
?
bin/webpack-dev-server.js
Outdated
@@ -337,7 +337,11 @@ function processOptions(wpOpt) { | |||
options.disableHostCheck = true; | |||
|
|||
if(argv["open"] || argv["open-page"]) { | |||
options.open = true; | |||
if(argv["open"] && argv["open"] !== "") { |
There was a problem hiding this comment.
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?
bin/webpack-dev-server.js
Outdated
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"] !== "") { | ||
open(uri + options.openPage, { app: options.open }).catch(function() { |
There was a problem hiding this comment.
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.");
});
If you call —open without a value argv["open“] will contain an empty string and options.open is going to be undefined.
Currently I don’t know if this can be managed somewhere else..
… Am 11.07.2017 um 17:48 schrieb Andrew Powell ***@***.***>:
@shellscape commented on this pull request.
In bin/webpack-dev-server.js <#825 (comment)>:
> options.open = true;
- options.openPage = argv["open-page"] || "";
+ options.openPage = argv["open-page"];
+ }
+
+ if(typeof argv["open"] !== "undefined") {
why not
if(argv["open"]) {
options.open = argv["open"] || true;
}
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#825 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMvJZksMcp_dOe6I8zOOArmfJGsk4A0Dks5sM5kygaJpZM4MSo_g>.
|
@shuijingleihen pull request discussions are not forums for support. Please ask questions on StackOverflow or the webpack Gitter (https://gitter.im/webpack/webpack). |
@shellscape Oh sorry, I thought it was an issue, thank you for your reminding! |
Any progress/changes on this PR? I'd love to help! |
@tomchentw I think we're good. @frankwinter needs to resolve the conflicts that popped up. I'll be on vaca until 8/27 but can merge when I'm back. |
WOW! |
devServer: { is working! thanks. it should giving example of allow to use string and Boolean :) |
This solves the issue. In webpack.config.js:
|
How can I open Firefox Developer Edition with the |
Fixes #753
An additional parameter for the
--open
option will pass it toopn
. For examplewebpack-dev-server --open 'Google Chrome'
will open chrome browser. When the parameter is omitted the systems default browser starts up as usual.