feat(open): modify `devServer.options.open` to take an object #1770

dangreenisrael · 2019-04-08T18:42:41Z

This PR updates devServer.options to take an array, making it consistent with the underlying library, open

https://www.npmjs.com/package/open

For Bugs and Features; did you add new tests?

I didn't see any tests for the unmodified code, I did however update the options.json schema

Motivation / Use-Case

This updates devServer.options to match the underlying library that is used in the implementation.

#1769

Breaking Changes

N/A

Additional Info

N/A

jsf-clabot · 2019-04-08T18:42:43Z

All committers have signed the CLA.

codecov · 2019-04-08T21:31:17Z

Codecov Report

Merging #1770 into master will increase coverage by 0.36%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1770      +/-   ##
==========================================
+ Coverage   92.42%   92.79%   +0.36%     
==========================================
  Files          25       25              
  Lines         964      971       +7     
  Branches      307      309       +2     
==========================================
+ Hits          891      901      +10     
+ Misses         70       67       -3     
  Partials        3        3

Impacted Files	Coverage Δ
lib/utils/runOpen.js	`94.11% <100%> (+24.11%)`	⬆️
lib/utils/colors.js	`60% <0%> (+10%)`	⬆️

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 6973e4c...37f3e88. Read the comment docs.

hiroppy

Need tests.

dangreenisrael · 2019-04-08T22:47:15Z

Hey @hiroppy. I added some tests for the config, but I didn't see anything in there testing the status file in general. Did you want me to add a suite for it, or did I miss something?

Thanks for helping maintain such an awesome project!!!

alexander-akait

Looks very good, we need tests, just mock opn package and check add arguments were passed in constructor of opn

alexander-akait · 2019-04-16T19:02:49Z

/cc @dangreenisrael friendly ping

dangreenisrael · 2019-04-17T12:19:45Z

Hey @evilebottnawi. We wound up not auto-open in the end so I'm not able to do it at work, and I don't have much extra time right now (I've got a 14 month old). That said, my plan is to try to get the tests written soon so this can land.

Sorry for the delays

alexander-akait · 2019-04-23T09:59:21Z

test/Status.test.js

+const log = {
+  info: jest.fn(),
+  warn: jest.fn(),
+};


Can we use just.spyOn to better reading tests, thanks!

We need to create log anyway. Would you prefer:

const log = { info: () => {} warn: () => {} } const infoSpy = jest.spyOn(log, 'info') const warnSpy = jest.spyOn(log, 'warn')

alexander-akait

One note and we can merge, thanks for PR

alexander-akait · 2019-04-23T11:53:26Z

lib/utils/status.js

@@ -46,7 +46,7 @@ function status(uri, options, log, useColor) {
    let openOptions = {};
    let openMessage = 'Unable to open browser';

-    if (typeof options.open === 'string') {
+    if (typeof options.open === 'string' || Array.isArray(options.open)) {


hm, why is array, based on docs https://github.com/sindresorhus/open it should be object

Options is an object, which takes app. App takes either a string or an array. https://www.npmjs.com/package/open#usage . This keeps with the existing API of this project, passing only the app value for open in options.

i.e:

{ open: "google chrome" }

and

{ open: ['google chrome', '--incognito'] }

Developers can want to use other options example: {wait: true}, also in future package can accept more arguments

Good call @evilebottnawi

I should have some time tomorrow night to work on this again

alexander-akait · 2019-04-23T11:53:37Z

lib/options.json

+        {
+          "type": "array",
+          "items": {
+            "type": "string"


items, can be not only strings

According to this, I believe they are only string https://www.npmjs.com/package/open#usage

{wait: true}

alexander-akait

I think we should do this in other way.

Let's make open will be Boolean, String or Object

if (Object.isObject(this.open)) {
  openOptions = Object.assign({}, options.open);
}

dangreenisrael · 2019-04-23T14:37:20Z

I think we should do this in other way.

Let's make open will be Boolean, String or Object
if (Object.isObject(this.open)) {
  openOptions = Object.assign({}, options.open);
}

I'm pretty sure the value of open's options.app which is what we are currently accepting in our options.open can be one of String, or Array<String>. Then if the user passes Boolean we do not pass in options

dangreenisrael · 2019-05-04T18:46:29Z

@evilebottnawi Based on what you requested (or my understanding of it), I made the following update:

Allow for 3 options to pass as options.open:

Boolean: runs without any options and thus uses the default browser
String: opens with the browser of your choice
Object: takes a settings object as defined by https://github.com/sindresorhus/open

As we are now doing now allowing for more complex (and possibly confusing) options to be passed in I have added logging aimed at making this more discoverable to a user.

If they pass in an object with neither app nor wait OR something that is not (boolean|string|object) they will get a warning telling them that they passed in an invalid config and a link to the open docs.

With that, I have also hard coded the open failure message, as config related info should now be caught earlier

dangreenisrael · 2019-05-04T23:26:54Z

If you guys are +1 to this I’ll update the tests and relevant docs

dangreenisrael · 2019-05-14T16:54:53Z

Bump

hiroppy · 2019-05-14T18:58:50Z

CI fails

Error: expect(jest.fn()).toBeCalledWith(expected)

Expected mock function to have been called with:
  {"app": ["Google Chrome", "incognito"]}
as argument 2, but it was called with
  ["Google Chrome", "incognito"].

Difference:

  Comparing two different types of values. Expected object but received array.
    at Object.toBeCalledWith (/Users/vsts/agent/2.150.3/work/1/s/test/Status.test.js:58:20)
    at Object.asyncJestTest (/Users/vsts/agent/2.150.3/work/1/s/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:102:37)
    at e (/Users/vsts/agent/2.150.3/work/1/s/node_modules/jest-jasmine2/build/queueRunner.js:43:12)
    at mapper (/Users/vsts/agent/2.150.3/work/1/s/node_modules/jest-jasmine2/build/queueRunner.js:26:19)
    at promise.then (/Users/vsts/agent/2.150.3/work/1/s/node_modules/jest-jasmine2/build/queueRunner.js:73:41)

alexander-akait · 2019-05-14T18:59:27Z

/cc @dangreenisrael need fix test problem

dangreenisrael · 2019-05-15T13:20:54Z

I just wanted a +1 to the approach before I took the time to update the tests. I should have some time to get it done this weekend

alexander-akait · 2019-05-15T13:26:29Z

@dangreenisrael +1

hiroppy · 2019-05-26T21:37:18Z

@dangreenisrael friendly ping

dangreenisrael · 2019-05-27T13:22:04Z

I haven't forgotten about you guys. Just super busy.

Thanks for the friendly ping tho

hiroppy · 2019-06-01T19:03:48Z

@evilebottnawi I updated this pr.

I'll do below after merging.

modify test/status
open has many keys
- https://github.com/sindresorhus/open#options
- we consider accepting an object or array

https://www.npmjs.com/package/open Allow for 3 options to pass as options.open: - Boolean: runs without any options and thus uses the default browser - String: opens with the browser of your choice - Object: takes a settings object as defined by https://github.com/sindresorhus/open As we are now doing now allowing for more complex (and possibly confusing) options to be passed in I have added logging aimed at making this more discoverable to a user. If they pass in an object with neither `app` nor `wait` OR something that is not ( boolean | string | object ) they will get a warning telling them that they passed in an invalid config and a link to the `open` docs. With that, I have also hard coded the open failure message, as config related info should now be caught earlier

hiroppy · 2019-06-01T21:09:11Z

need to update documentation (but I want to consider open types)

…er.open

alexander-akait

Looks good, one note

alexander-akait · 2019-06-03T12:20:45Z

lib/utils/runOpen.js


  if (typeof options.open === 'string') {
    openOptions = { app: options.open };
-    openMessage += `: ${options.open}`;
+  } else if (typeof options.open === 'object') {


Maybe add Array.isArray?

#1770 (review)

dangreenisrael · 2019-06-17T23:57:31Z

Is this still something people are still interested in. If so, can we agree on an interface in principle before any more code changes?

alexander-akait

Two notes

alexander-akait · 2019-06-24T09:18:11Z

test/Status.test.js

+      expect(open).toBeCalledWith(uri, { app: ['Google Chrome', 'incognito'] });
+    });
+  });
+});


Please move this tests to https://github.com/webpack/webpack-dev-server/blob/master/test/server/open-option.test.js

alexander-akait · 2019-06-24T09:18:40Z

lib/utils/runOpen.js

+    openOptions = options.open;
+    if (!options.app && !options.wait) {
+      handleInvalidOptionsConfig(log, options.open);
+    }


Why we need this? open package can add more option in future

alexander-akait · 2019-08-20T14:14:28Z

/cc @dangreenisrael friendly ping

dangreenisrael · 2019-08-20T14:46:07Z

Wow, totally forgot about this. I can try to look into in the next few weeks. If someone else wants to take it over I promise I won't be offended

stefan-winkler-diconium · 2020-02-04T12:24:07Z

@dangreenisrael friendly ping - It would be super-useful to finally have proper "open" support for webpack. Do you think you can wrap this up soon?

alexander-akait · 2020-02-04T12:25:24Z

@stefan-winkler-diconium you can send another PR and fix problems described here

dangreenisrael · 2020-02-04T13:56:11Z

I've got a broken thumb, so I won't be doing any non-work required coding for at least a month. Sorry

dangreenisrael requested review from alexander-akait and hiroppy as code owners April 8, 2019 18:42

dangreenisrael force-pushed the #1769-Allow-passing-args-to-devServer.open branch from ad6d8c7 to 7b0e401 Compare April 8, 2019 21:31

hiroppy requested changes Apr 8, 2019

View reviewed changes

dangreenisrael force-pushed the #1769-Allow-passing-args-to-devServer.open branch from 7b0e401 to ec7cb84 Compare April 8, 2019 21:51

alexander-akait reviewed Apr 9, 2019

View reviewed changes

hiroppy added the need tests label Apr 19, 2019

alexander-akait reviewed Apr 23, 2019

View reviewed changes

alexander-akait requested changes Apr 23, 2019

View reviewed changes

alexander-akait approved these changes Apr 23, 2019

View reviewed changes

alexander-akait reviewed Apr 23, 2019

View reviewed changes

alexander-akait requested changes Apr 23, 2019

View reviewed changes

hiroppy removed the need tests label Apr 23, 2019

dangreenisrael force-pushed the #1769-Allow-passing-args-to-devServer.open branch 3 times, most recently from 2096bc1 to 7b0560b Compare May 4, 2019 18:42

alexander-akait previously approved these changes May 14, 2019

View reviewed changes

hiroppy dismissed alexander-akait’s stale review via de6aad9 June 1, 2019 19:00

hiroppy force-pushed the #1769-Allow-passing-args-to-devServer.open branch 2 times, most recently from de6aad9 to 0ec71b3 Compare June 1, 2019 19:00

hiroppy force-pushed the #1769-Allow-passing-args-to-devServer.open branch from 0ec71b3 to 8ff23b7 Compare June 1, 2019 19:06

hiroppy changed the title ~~Update devServer.options to take an array~~ feat(open): modify devServer.options.open to take an object Jun 1, 2019

hiroppy force-pushed the #1769-Allow-passing-args-to-devServer.open branch from 8ff23b7 to f4e919b Compare June 1, 2019 20:24

refactor: rebased

a3b06dc

hiroppy force-pushed the #1769-Allow-passing-args-to-devServer.open branch from f4e919b to a3b06dc Compare June 1, 2019 20:37

hiroppy closed this Jun 1, 2019

hiroppy reopened this Jun 1, 2019

hiroppy approved these changes Jun 1, 2019

View reviewed changes

Merge branch 'master' into webpack#1769-Allow-passing-args-to-devServ…

37f3e88

…er.open

alexander-akait requested changes Jun 4, 2019

View reviewed changes

alexander-akait requested changes Jun 24, 2019

View reviewed changes

EslamHiko mentioned this pull request Apr 1, 2020

feat: allow open option to accept an object #2492

Merged

6 tasks

hiroppy closed this Apr 2, 2020

feat(open): modify devServer.options.open to take an object #1770

feat(open): modify devServer.options.open to take an object #1770

Conversation

dangreenisrael commented Apr 8, 2019

For Bugs and Features; did you add new tests?

Motivation / Use-Case

Breaking Changes

Additional Info

jsf-clabot commented Apr 8, 2019 • edited Loading

codecov bot commented Apr 8, 2019 • edited Loading

Codecov Report

hiroppy left a comment

Choose a reason for hiding this comment

dangreenisrael commented Apr 8, 2019

alexander-akait left a comment

Choose a reason for hiding this comment

alexander-akait commented Apr 16, 2019

dangreenisrael commented Apr 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

dangreenisrael commented Apr 23, 2019 • edited Loading

dangreenisrael commented May 4, 2019 • edited Loading

dangreenisrael commented May 4, 2019 • edited Loading

dangreenisrael commented May 14, 2019

hiroppy commented May 14, 2019

alexander-akait commented May 14, 2019 • edited Loading

dangreenisrael commented May 15, 2019

alexander-akait commented May 15, 2019

hiroppy commented May 26, 2019

dangreenisrael commented May 27, 2019

hiroppy commented Jun 1, 2019

hiroppy commented Jun 1, 2019 • edited Loading

alexander-akait left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dangreenisrael commented Jun 17, 2019

alexander-akait left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait commented Aug 20, 2019

dangreenisrael commented Aug 20, 2019

stefan-winkler-diconium commented Feb 4, 2020

alexander-akait commented Feb 4, 2020

dangreenisrael commented Feb 4, 2020

feat(open): modify `devServer.options.open` to take an object #1770

feat(open): modify `devServer.options.open` to take an object #1770

jsf-clabot commented Apr 8, 2019 •

edited

Loading

codecov bot commented Apr 8, 2019 •

edited

Loading

dangreenisrael commented Apr 23, 2019 •

edited

Loading

dangreenisrael commented May 4, 2019 •

edited

Loading

dangreenisrael commented May 4, 2019 •

edited

Loading

alexander-akait commented May 14, 2019 •

edited

Loading

hiroppy commented Jun 1, 2019 •

edited

Loading