Allow Open -o with both relative & absolute paths

[lski]

Allows the use of both absolute and relative paths when using the open flag [-o]. This is a none breaking change as currently the Open option just doesnt work (in my tests) if starting with a forward slash.

Allowing absolute paths is important as someone might want to force the use of a particular host that doesnt want to use the value in canonicalHost.

It determines whether it is a relative path or not simply by checking whether the -o argument starts with a forward slash, anything else passes through as before.

[craigmichaelmartin]

I'm not sure if @indexzero is interested in this, but squashing your commits would help your case :)

Also, this looks like a duplicate of #234 which was opened a few months before this PR. I would recommend closing this and code reviewing #234 if you think this PR has something that was missed.

[lski]

I know about #234 but he introduced a breaking change, my pull request does something similar but is a none breaking change to existing peoples code. (Not criticising their code)

In their solution everybody would be forced to change any existing code that works with the current implementation to switch from absolute to relative paths. I have given an alternative that introduces relative paths but retains absolute paths. This can be an issue where people have hard coded the absolute path in say an npm script. Hence why the additional pull request.

As for the squashing commits, I was not aware you could do that after submitting, it failed only on code style guidance, the same as hancharou did. If it is possible then I am happy to do it, or do you mean before I re-commited the style changes in the first place? I have done years of private company source control work before, but this is in fact my first open source pull request, so I am open to suggestions.

[craigmichaelmartin]

Cool, thanks for articulating the difference!

You can check out https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History for squashing. In your case git rebase -i HEAD~3 should do the trick. It'll open a text editor. Change the second and third pick to s and then save. It'll send you back to the command line with directions on how to amend the commit (you may want to change the message), and then git rebase --continue. In order to push back to this branch, you will need to add a -f.

[lski]

Thank you for the link, I cant do it this minute Ill read through that particular help page properly before I do it, your example makes sense, but will want to make sure I dont do something stupid (the git help pages are great but wow its easy to miss alot when you dont know exactly what your looking for). Thank you again :)

[igoncharov85]

Lski,
I tested your solution on Windows 8.1 x64 and see my results:
http-server -o index.html index.html is opened in IE and it's not served through HTTP. I see local path in IE.
http-server -o /index.html index.html is opened and served through HTTP.
http-server -o folder/2.html 2.html is opened in IE and it's not served through HTTP. I see local path in IE.
http-server -o /folder/2.html 2.html is opened and served through HTTP.

My #234 serves all commands correctly and serves all of them through HTTP. This change will open files properly not matter you provide it with / or without. I've squashed my commit into one so it can be merged in on commit.

[lski]

Sorry but I disagree, I would suggest "index.html" is still a relative path, "http://127.0.0.1:8081/index.html" is an absolute path and what people are probably currently using.

Your pull request does not allow absolute paths to be loaded any more which could BREAK their existing build scripts.

Although just my opinion I would say starting with a forward "/" slash makes sense with regards to the web as its a root relative from the website you have just started.

I am happy to switch my code to sniff for http/https if no slash is really that important and what people want, Im flexible :) or if not, please update your PR so that it doesnt break peoples existing code.

[igoncharov85]

lski,
I disagree with you. I think if you provide -o index.htmlor -o /index.htmlthe result should be the same. In both cases you expect to see http://127.0.0.1:8080/index.html

What do you mean by relative path? You still will be opening root + whatever you have in -o parameter. For me it's even more obvious to have -o /index.html so I know that it'll open the file from root folder.

[lski]

Ok lets step back from 'relative' and 'absolute' path as it s not so important and focus on the fact your proposing a breaking change. Imagine the following snippet from someones existing packge.json:

{
    "scripts": {
         "build": "http-server -p 8081 -o http://myComputerName/subfolder/index.htm"
    }
}

In that example it would turn out "http://127.0.0.1/http://myComputerName/subfolder/index.htm" which obviously wont work.

You are also making the assumption that people will always want to use "http://127.0.0.1/subfolder/index.html" so will be happy with "http://127.0.0.1/" + whatever they put, but sometimes that just is not what people need to load.

The example above of "computer name" host instead of 127.0.0.1 could be because they are using cors with a base api service and need to ensure they are coming from a particular host.

Hopefully that makes more sense :)

[lski]

Correction "http://myComputerName:8081/subfolder/index.htm" as I missed the port number (sorry on mobile this week). And I meant to mention a local network address IP e.g. "http://192.168.0.1:8081/subfolder/index.htm"

[craigmichaelmartin]

Why do you keep this line opener(baseUrl, { command: argv.o !== true ? argv.o : null }); as is? Is not passing opener a command with anything other than null undesired?

[lski]

Well the check in the original is there because if you pass "non-string" values to the opener via the arguments e.g. true, 2 etc it throws an error, so the check needs to be there. And a check still needs to be made after checking the initial check for the "/", because if it is a string value e.g. a full url "http://127.0.0.1:8080/" it still needs to be passed to behave the same as it did before this change.

I believe the author originally passed the command separately as if options.command == null then it will use the first argument, otherwise uses the options.command. I decided to stick with the authors original way as to make it more likely they would be happy with the pull request.

Although I should have changed it to the following to avoid other values other than just 'true':

opener(typeof argv.o === 'string' ? argv.o : protocol + '//' + canonicalHost + ':' + port);

When doing the code I originally wrote something along the lines of the code below:

if (argv.o) {
  var isStr = (typeof argv.o === 'string');
  if (isStr && argv.o.charAt(0) === '/') {
    opener(protocol + '//' + canonicalHost + ':' + port + argv.o);
  }
  else {
    opener(isStr ? argv.o : protocol + '//' + canonicalHost + ':' + port);
  }
}

But I felt I then possibly altering the behaviour of the code, so I put the original line back in. I personally would have preferred to use the my original code as it was easier to read for me and did a check for string values.

[pmunin]

any update on this issue?

[andrevenancio]

any updates on this? Its been a while :) close it or merge it? 😆

[BigBlueHat]

@andrevenancio I'm not at all opposed to merging this (or one of the similar duplicate attempts at solving it), but (as you've noted) it's been a while, so this (and/or the other approaches) need review/comparison/tests/etc.

If you want to help dig through the other related opener issues and make a recommendation (and ideally tests!), then I'm more than happy to review it and (if all is well) merge it.

[andrevenancio]

@lski do you have time to add a test since you're more into this issue originally?

Also, would this make everybody happy:

if (argv.o) {
    opener(argv.o);
}

so things like:
http-server -o ./build/index.html
http-server -o http://localhost:8888/example

will work making everybody happy? 😁

[lski]

@andrevenancio I am currently travelling without a laptop, so not able to atm, sorry

[thornjad]

I believe this should be achieved by the earlier #250. Please comment and/or reopen if this is not the case.