Allow The Lounge to be proxied behind a /path/ url

[gdamjan]

contains three small changes to use relative urls instead of absolute rooted at /

• favicons in index.html
• badge and pop sound in lounge.js
• the url for the socket.io endpoint

Typical use with nginx:

  location /path/ {
    proxy_http_version 1.1;
    proxy_set_header Upgrade $http_upgrade;
    proxy_set_header Connection "upgrade";
    proxy_pass http://localhost:9000/;
  }

those trailing / are important.

[astorije]

FYI, this is a rework of #3 and #20 (after it was reverted in #21).

[astorije]

As mentioned on the IRC channel, I have concerns about this change.
In particular, we should completely ensure that the existing solution fully works after your change.
So that would be great if someone could test running The Lounge under the following schemes:

• http://domain.com/lounge/
• http://lounge.domain.com/
• http://domain.com:9000/

(Checking these as per @JocelynDelalande's tests)

[xPaw]

@astorije pathname would be as follows:

1. /lounge, and in this case, yes, it will break.
2. /
3. /

[astorije]

Indeed, but we cannot release this without actually testing, with the risk of breaking The Lounge for everyone as #3 / #20 did.

[gdamjan]

/lounge will break with or without my patch, ain't it?

[astorije]

But I'm confused @gdamjan, wasn't that the purpose of your PR, to run The Lounge behind a /whatever path?

[gdamjan]

huh. ok. I'm not sure what you were asking, but the very important difference is /path vs /path/ (as I've specified in the commit message).

[astorije]

Right, I see, sorry. I'm ok with the distinction, and wherever this gets documented or used, I'd suggest to add a redirect from /path to /path/ if possible.
Editing my checklist above.

[gdamjan]

with the above nginx rule, it'll send a redirect automatically.

[astorije]

Good to know, thanks!

[gdamjan]

and to be extra clear... both of the trailing slashes are important, in location and in proxy_pass.

[astorije]

Adding help wanted label, if someone can test the 3 scenarios listed above and report that would help shipping this faster. Thanks!

[JocelynDelalande]

Tested the 3 scenarios and read patch, it's ok

👍 :)

[astorije]

Letting @YaManicKill give his second review to this one as he self-assigned #3 at the time.

[AlMcKinlay]

So, on the basis of @JocelynDelalande having tested this, and it looks good (and solves the issue we had last time we merged it) I'll give my 👍 and merge.

[astorije]

Great, really nice addition! I'll publish to npm either today or tomorrow!
(Also, removing second review needed label)

[AlMcKinlay]

(Also, removing second review needed label)

Sorry, I'll get the hang of this soon. This is the biggest project I've maintained :-P