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

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

merged 18 commits into from
Aug 30, 2017

Conversation

frankwinter
Copy link
Contributor

@frankwinter frankwinter commented Mar 3, 2017

Fixes #753

An additional parameter for the --open option will pass it to opn. For example webpack-dev-server --open 'Google Chrome' will open chrome browser. When the parameter is omitted the systems default browser starts up as usual.

@jsf-clabot
Copy link

jsf-clabot commented Mar 3, 2017

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Mar 11, 2017

Codecov Report

Merging #825 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf5dda8...a230754. Read the comment docs.

if(typeof options.open === "string")
params.push({ app: options.open });

open(...params).catch(function() {
Copy link
Member

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.

Copy link
Member

@SpaceK33z SpaceK33z left a 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.

@sergeymorkovkin
Copy link

Any chances to have it merged? :)

@shellscape
Copy link
Contributor

shellscape commented Jul 10, 2017

@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.

@sergeymorkovkin
Copy link

@shellscape Got it. Thanks for an update. If you think I could do this, please let me known.

Copy link
Contributor

@shellscape shellscape left a 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">
Copy link
Member

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/*


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

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

@shellscape
Copy link
Contributor

@frankwinter you'll have to remove the package-lock.json as well.

@frankwinter
Copy link
Contributor Author

Sorry, please excuse the clumsy workflow.. It's my first contribution on a forked github-project :-/

Copy link
Contributor

@shellscape shellscape left a 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

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")?

@@ -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?

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() {
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.");
});

@frankwinter
Copy link
Contributor Author

frankwinter commented Jul 11, 2017 via email

@mirages
Copy link

mirages commented Aug 8, 2017

How to set the parameter in webpack.config.js file? My configure likes that:

devServer: {
    port: '8081'
    ,compress: true
    ,watchContentBase: true
    ,open: 'chrome'
    ,openPage: 'views/index/'
}

but it doesn't work!!! And throw an error:

tim 20170808163019

@shellscape
Copy link
Contributor

@shuijingleihen pull request discussions are not forums for support. Please ask questions on StackOverflow or the webpack Gitter (https://gitter.im/webpack/webpack).

@mirages
Copy link

mirages commented Aug 8, 2017

@shellscape Oh sorry, I thought it was an issue, thank you for your reminding!

@tomchentw
Copy link

Any progress/changes on this PR? I'd love to help!

@shellscape
Copy link
Contributor

@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.

@shellscape shellscape dismissed SpaceK33z’s stale review August 30, 2017 02:01

changes made as requested

@shellscape shellscape changed the base branch from master to abandoned-pull-requests August 30, 2017 02:06
@shellscape shellscape changed the title Add optionally browser parameter to --open Allow --open option to specify the browser to use Aug 30, 2017
@shellscape shellscape changed the base branch from abandoned-pull-requests to master August 30, 2017 02:08
@shellscape shellscape merged commit f00fcb3 into webpack:master Aug 30, 2017
@tomchentw
Copy link

WOW!

@frankwinter frankwinter deleted the open-specific-browser branch August 30, 2017 17:26
@zigang93
Copy link

zigang93 commented Jan 9, 2018

devServer: {
port: '8081'
,compress: true
,open: 'firefox'
}

is working! thanks.
Btw, should we update the doc on https://webpack.js.org/configuration/dev-server/#devserver-open ?

it should giving example of allow to use string and Boolean :)

@crowscript
Copy link

This solves the issue.
In package.json:
"start": "webpack-dev-server --config webpack.dev.js --open chrome",

In webpack.config.js:

devServer: {
        ...
        open: 'chrome'
      },

@jdavidferreira
Copy link

How can I open Firefox Developer Edition with the open property?

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.

Specifying browser for --open flag