Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for brotli encoding #463

Merged
merged 6 commits into from
Apr 15, 2019
Merged

Conversation

thornjad
Copy link
Member

@thornjad thornjad commented Sep 3, 2018

As requested in #445, this adds support for serving brotli-encoded files by passing --brotli or -b to http-server. This necessitates bumping up the minimum ecstatic dependency version to 3.3.0, but this shouldn't have any breaking changes.

@filipesilva
Copy link

@indexzero @BigBlueHat anything I can help out with to get this merged?

I tested it locally by following these steps:

I think this PR is working as intended 👍

@starpit starpit mentioned this pull request Sep 12, 2018
@BigBlueHat BigBlueHat added the minor version non-breaking, non-trivial change label Oct 4, 2018
@filipesilva
Copy link

Heya, don't mean to be a bother but just wanted to ping about this again since there's a project I'm working on that's waiting for this support. Keep up the good work!

@thornjad
Copy link
Member Author

@BigBlueHat do you have the ability to merge? And if so, what do you think?

@BigBlueHat
Copy link
Member

@thornjad sorry I hadn't responded earlier other than the labeling.

Since this bumps the underlying node-ecstatic to v3.3.0, I want to be sure that doesn't have any unexpected consequences. IIRC there was some significant shift in the MIME libraries in use which may effect custom type loading for some folks.

We've also got a bit of a tangle with #464, but since you're the author of both, maybe you can help queue those in your preferred order? 😃

Regardless, I do want to get this merged--and ideally soon.

We should also get some tests written for the .br and the priority order thing around .br priority vs. .gzip.

@thornjad
Copy link
Member Author

Hey, that's alright.

Tests are a good idea. I did put some in ecstatic, so we are partially covered that way, but http-server should be properly testing itself. I actually keep a sticky note on my desk that just reads "You need more tests".

For the MIME libraries, tests would probably be good here as well, but I've never used custom types in real life, so I'm not sure what to look for.

As for #464, either PR can go first and the other can be updated to match. This PR is a more meaningful change, so maybe this one should go first.

@BigBlueHat
Copy link
Member

@thornjad wrt to mime stuff, I may be remembering this all wrong. It looks like #35 is a pending request which has an old PR #152 which may be obviated by node-ecstatic's direct mime type customization.

PR #472 exposes the raw JSON mime config to the command line, but doesn't handle the .types loading which node-ecstatic makes available as a path set on --mime-types.

In short, this is probably not a blocker. 😄 But it is something there's long standing desire for, so maybe we tackle that once much of this is in place.

With some basic tests for this code, I'd be say it's ready to go (it reads well anyway).

Thanks for the help @thornjad!

@thornjad
Copy link
Member Author

Sorry for being ridiculously slow, I've been atypically busy. I've added tests for compression.

I'll update #464 after this merges!

@wentjun
Copy link

wentjun commented Feb 28, 2019

Hi! I have been following this issue for a while. Sorry for asking this but.. May I know when will this be merged/released?

@thornjad
Copy link
Member Author

@wentjun sorry for the delay! I am working on a giant merge train so this should be merged soon!

@thornjad thornjad added this to the v0.12.0 milestone Feb 28, 2019
@thornjad thornjad merged commit e1f5d23 into http-party:master Apr 15, 2019
thornjad added a commit that referenced this pull request Apr 15, 2019
...coding

Add support for brotli encoding
@thornjad thornjad deleted the brotli-support branch April 15, 2019 17:20
@filipesilva
Copy link

Awesome, happy to see this go in!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor version non-breaking, non-trivial change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants