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 'br' encoding #333

Closed
wants to merge 1 commit into from
Closed

Add 'br' encoding #333

wants to merge 1 commit into from

Conversation

silverwind
Copy link

Noticed that the greatest encoding out there was missing from this list.

@silverwind
Copy link
Author

Also, according to https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Encoding, there's a possibilty for compress encoding, but I don't think this is worth adding here.

@kevva
Copy link
Contributor

kevva commented Jun 13, 2017

Is it worth adding a test for this maybe? Could use https://www.npmjs.com/package/brotli.

@silverwind
Copy link
Author

That would add a devDep on https://github.com/mayhemydg/iltorb, which requires a native module. I don't think it's worth that as gzip is already tested.

@kevva
Copy link
Contributor

kevva commented Jun 13, 2017

How do we decompress the br content? Seems like zlib (which we are using in decompress-response) doesn't.

@silverwind
Copy link
Author

How do we decompress the br content?

iltorb can both compress and decompress.

@kevva
Copy link
Contributor

kevva commented Jun 13, 2017

Yeah, but I mean when using got and you try to fetch br encoded content, it wouldn't be decompressed if decompress is true. We're only handling gzip and deflate I think. Not that it affects this PR, but it might be worth handling it?

@silverwind
Copy link
Author

Oh, right. Looks like https://github.com/sindresorhus/decompress-response is lacking support for br. I guess it should be added there first.

@kevva
Copy link
Contributor

kevva commented Jun 13, 2017

The module I linked further up should handle it without native deps I think, https://github.com/devongovett/brotli.js.

@silverwind
Copy link
Author

Haven't seen that one yet, but iltorb is well maintained and I haven't had any issues with it yet, even under Windows. Brotli compression is very CPU intensive, so I'd definitely compare the performance before choosing a module.

@kevva
Copy link
Contributor

kevva commented Jun 13, 2017

Yup, I'll work on a PR, but I'm willing to sacrifice performance over native deps tbh depending on the impact.

@silverwind
Copy link
Author

Opened sindresorhus/decompress-response#12 which blocks this issue.

@sindresorhus
Copy link
Owner

@sindresorhus
Copy link
Owner

Node.js issue about adding it natively: nodejs/node#18964

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants