Add support for brotli encoding

[thornjad]

Brotli encoding is a relatively new compression algorithm which may be better than gzip, and is supported by all major browsers according to Wikipedia. This patch adds support for ecstatic to serve brotli-encoded files (using the .br extension) if the browser supports it.

Some things to note:

• Firefox and Chromium, and probably other browsers, only support brotli when using https, except that Chromium allows it from localhost, so it can be tested by running locally.
• I am unaware of any method for testing if a file is in fact brotli-encoded (as opposed to simply having a .br extension) like the way we can check for gzip encoding. If anyone knows how to do this, it should be added.
• As I've written it here, ecstatic by default tries to serve brotli, next tries gzip, then falls to un-encoded. I've set brotli on by default and ahead of gzip because it looks like brotli performs better (see article linked above).

[dotnetCarpenter]

@thornjad pretty sure adding fs.stat calls before responding will make ecstatic slower, regardless of the optimized encoding. Perhaps this should be opt-in?

[dotnetCarpenter]

ahh you did: https://github.com/jfhbrook/node-ecstatic/pull/228/files#diff-b2b5a88fb51675f1aa1065c093dce1eeR451

[jfhbrook]

I believe the stat call currently happens with gzip, fwiw

[thornjad]

Yes, I modeled it on the way gzip was already implemented, and only making the fs.stat calls when necessary. If there's a faster way to do the same thing, however, I'm all in.

[jfhbrook]

possibly relevant https://github.com/jfhbrook/node-ecstatic/pull/195/files

[thornjad]

How does this look to everyone? Are there any objections?

[jfhbrook]

No immediate concerns, but I'd like to take a closer look and kick the tires on this just the same

[jfhbrook]

Hey, so I did a real review and this looks pretty good! I want to make the brotli encoding off by default so as to not change the default API contract, but that's a pretty minor thing. We'll need documentation as well.

If you can do these things, great! If not, let me know and I can probably push this the rest of the way.

Thanks for your work on this! Neat feature.

[thornjad]

Documentation added! Thank you for reviewing!

[jfhbrook]

I'll ship this later tonight!

[jfhbrook]

Hey, so uh, after merging this I get multiple test fails on Windows @thornjad , can you reproduce (on any platform, maybe not win-specific) ? I can track down this failure but not right now, maybe in the next day or two, but knowing if it's just me will help.

[jfhbrook]

test/compression.js ................................. 16/18
  serves gzip-encoded file when brotli not accepted
  not ok should be equal
    +++ found
    --- wanted
    -"gzip"
    +[null]
    compare: ===
    at:
      line: 91
      column: 9
      file: test/compression.js
      type: Request
      function: request.get
      method: _callback
    stack: |
      Request.request.get [as _callback] (test/compression.js:91:9)
      Request.self.callback (node_modules/request/request.js:185:22)
      Request.<anonymous> (node_modules/request/request.js:1161:10)
      IncomingMessage.<anonymous> (node_modules/request/request.js:1083:12)
    source: |
      t.equal(res.headers['content-encoding'], 'gzip');

  serves gzip-encoded file when brotli not enabled
  not ok should be equal
    +++ found
    --- wanted
    -"gzip"
    +[null]
    compare: ===
    at:
      line: 121
      column: 9
      file: test/compression.js
      type: Request
      function: request.get
      method: _callback
    stack: |
      Request.request.get [as _callback] (test/compression.js:121:9)
      Request.self.callback (node_modules/request/request.js:185:22)
      Request.<anonymous> (node_modules/request/request.js:1161:10)
      IncomingMessage.<anonymous> (node_modules/request/request.js:1083:12)
    source: |
      t.equal(res.headers['content-encoding'], 'gzip');

[thornjad]

Very weird. Yes I can reproduce this on Linux, I'm going to see if I get the same on an entirely fresh install on Windows. The weird part is that I don't get the same test failures on my fork.

[jfhbrook]

ok word so it's probably not a windows-specific issue. You don't have to try to repro on windows specifically in that case, imo.

and yeah, frickin' weird right??

[thornjad]

Ready for a new kicker? I just pulled master into the master on my fork, and? No error. I'm going to try to investigate this now if I have enough time.

[jfhbrook]

wondering if it has to do with line endings

[jfhbrook]

y'know \n vs \r\n, git pulls some shenanigans on text files to make them happy this way

[jfhbrook]

also, check for accidentally uncommitted things, maybe you have a delta or a new file somewhere

[thornjad]

Nothing uncommitted. I've seen line endings messed up in git before, and it doesn't look like that's the case, but I don't know how to check for sure.

[jfhbrook]

I'm not 100% on that either, maybe a regexp search for \n vs \r\n ? or we use a binary upload/download site and run diff?

[thornjad]

With some experimentation, it looks like in those two test cases, where brotli is not accepted and where brotli is not enabled, it isn't falling back on gzip like it should. For example, when I go to test/public/brotli in firefox (which doesn't support brotli without https), I get the uncompressed index file.

As for differences, I used cat lib/ecstatic.js | grep "^M" and found nothing, and diff and comm find no differences with my fork. I can look more tomorrow, I really don't know what's up but really want to find out.

[jfhbrook]

Possible there's a race condition?

[thornjad]

I've found the problem, and it's frustratingly simple! Both of those tests are attempting to serve test/public/brotli/index.html.gz, which exist in my local fork. However, *.gz is in the .gitignore, so those local files were never even tracked. Since I added the entire test/public/brotli directory at once, I didn't even notice. Getting rid of that line in the .gitignore fixes it all!

Because of #230, how should I submit this change? Should it be a new PR?

[jfhbrook]

let's try this:

1. I create a branch that reverts the revert
2. you fork that with the fix
3. pr against my revert revert branch, I merge
4. then I merge the revert revert, and we're good

A little "hold my beer" but I think that'll have the correct behavior in terms of playing nice w/ git and github.

[jfhbrook]

Here's the branch: https://github.com/jfhbrook/node-ecstatic/tree/revert-230-revert-228-brotli-encoding

[jfhbrook]

You can pull this branch locally from my upstream, then cherry-pick the fix in and it should do the right thing

[jfhbrook]

Published 3.3.0, cheers!