--open-path and --open-app treated as strings, not JSON.

[benwiley4000]

This PR contains a:

• bugfix
• new feature
• code refactor
• test update
• typo fix
• metadata update

Motivation / Use-Case

Seems like we were JSON.parse ing the open.app and open.path string properties for no reason... maybe I missed something? All I know is that my server works now. 😄

Additional Info

I wanted to fix or update tests, but it seems the relevant test was failing for me before and after I made the change.

[jsf-clabot]

All committers have signed the CLA.

[codecov]

Codecov Report

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

@@           Coverage Diff           @@
##           master      #60   +/-   ##
=======================================
  Coverage   92.19%   92.19%           
=======================================
  Files           7        7           
  Lines         269      269           
=======================================
  Hits          248      248           
  Misses         21       21

Impacted Files	Coverage Δ
lib/options.js	87.8% <ø> (ø)	⬆️

---

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 a436807...55aeac1. Read the comment docs.

[shellscape]

Thanks for the PR 🍺

Which test was failing and why? That's a strange assertion as nothing would've passed CI previously if we had failing tests.

[benwiley4000]

@shellscape Strange indeed! 😄

$ npm install
# ...
$ npm test

Here is my output: https://hastebin.com/napesipuyo.coffeescript

This is macOS High Sierra, though it's similar to the result I got running on Linux earlier.

[benwiley4000]

The specific relevant test I was referring to is:

webpack-serve Options
    17) should accept an open:Object option

Though that's certainly not the only failing test.

[shellscape]

Yeah I'm on High Sierra here too, no issues with the tests.

At any rate, note the last example here: https://github.com/sindresorhus/opn#usage. That's why we're using JSON.parse, as the value can be an array for --open-app. But it does need some type checking if it's simply a string. --open-path doesn't need to be parsed, however.

So there's actually no tests for either option in the test/tests/cli.js file, which is where they'd need to be. I'd say your original PR then isn't complete, and you'll have to get those tests passing, plus new tests for both CLI options, before we could merge this. If you'd like to defer to us to fix this, we can probably get that in next week Monday or Tuesday.

[benwiley4000]

@shellscape I'm running the tests on the master branch, so I'm not sure what to make of this. Are you sure you don't have anything set up in your local environment that isn't dependent upon the repo? If you could verify in a new clone/maybe even a new machine that would be awesome, just so we can make sure it's me that is crazy. 😉

Re: the API, I think passing a JSON literal as a command line argument seems a bit fishy. Wouldn't it be better to make it a comma separated list -

webpack-server ... --open-path a,b,c

Then in the code you can just do openPath.split(',') and it works even if there are no commas.

What do you think?

[shellscape]

Are you sure you don't have anything set up in your local environment that isn't dependent upon the repo?

Yes. Each CI run is a fresh clone in an independent environment. There's something awry on your end I'm afraid.

What do you think?

I think no. If the user wants that advanced functionality, they can be explicit in what they need. We don't need to go creating any special rules around that.

[benwiley4000]

Yes. Each CI run is a fresh clone in an independent environment. There's something awry on your end I'm afraid.

It looks like your CI server is using a node module cache. Have you tried invalidating it? If that's not the issue, I'm officially out of ideas.. I've now cloned many times on two computers, multiple Wi-Fi networks, different Node versions, and I'm still unable to make the tests pass locally. Bummer! 😞

I think no. If the user wants that advanced functionality, they can be explicit in what they need. We don't need to go creating any special rules around that.

Fair enough. As long as I can use a plain string for --open-path, that's good with me! 🙂

[shellscape]

Thanks for your work on this. The fix needed quite a bit more additional work, so #72 was created. That PR supersedes this one, so we're going to close this one up. Look for this fix to be published in the next day or so.

[benwiley4000]

Thanks! 😄