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.

Sign up for GitHub

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

Jump to bottom

feat: add brotli to supported compression 🗜️ #4503

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat: add brotli to supported compression 🗜️ #4503

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ae83459
feat: add brotli to supported compression 🗜️
bricss May 19, 2024
ab0b6d4
chore: address comments 📝
bricss May 19, 2024
5466504
chore: address comments 📝
bricss May 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions lib/compression.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,21 +8,23 @@ const Hoek = require('@hapi/hoek');


const internals = {
common: ['gzip, deflate', 'deflate, gzip', 'gzip', 'deflate', 'gzip, deflate, br']
common: ['gzip, deflate', 'deflate, gzip', 'gzip', 'deflate', 'br', 'gzip, deflate, br']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addition does not seem useful. 'br' is quick to parse and I don't believe it is indeed a commonly used string.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it worth to have it for consistency? 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can remove br from commons, but by this logic it will make sense also to remove gzip, deflate as well 🤷

};


exports = module.exports = internals.Compression = class {

decoders = {
br: (options) => Zlib.createBrotliDecompress(options),
gzip: (options) => Zlib.createGunzip(options),
deflate: (options) => Zlib.createInflate(options)
};

encodings = ['identity', 'gzip', 'deflate'];
encodings = ['identity', 'gzip', 'deflate', 'br'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably appear before 'gzip'. Otherwise gzip will be preferred for all practical use-cases.

Copy link
Author

@bricss bricss May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, the order thingy should be addressed with compression.priority feature, which I plan to add if this one will land 🛬

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, the order should stay as is, bcos in current implementation gzip used by default when client sends accept-encoding: 'gzip, deflate, br', and changing the order will be a breaking change. Best way to address this is to add compression.priority feature to make it possible flexibly change the order of compression algorithms.


encoders = {
identity: null,
br: (options) => Zlib.createBrotliCompress(options),
gzip: (options) => Zlib.createGzip(options),
deflate: (options) => Zlib.createDeflate(options)
};
Expand All @@ -45,7 +47,6 @@ exports = module.exports = internals.Compression = class {

addEncoder(encoding, encoder) {

Hoek.assert(this.encoders[encoding] === undefined, `Cannot override existing encoder for ${encoding}`);
Hoek.assert(typeof encoder === 'function', `Invalid encoder function for ${encoding}`);
this.encoders[encoding] = encoder;
this.encodings.unshift(encoding);
Expand All @@ -54,7 +55,6 @@ exports = module.exports = internals.Compression = class {

addDecoder(encoding, decoder) {

Hoek.assert(this.decoders[encoding] === undefined, `Cannot override existing decoder for ${encoding}`);
Hoek.assert(typeof decoder === 'function', `Invalid decoder function for ${encoding}`);
this.decoders[encoding] = decoder;
}
Expand Down
4 changes: 3 additions & 1 deletion lib/types/server/encoders.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { createDeflate, createGunzip, createGzip, createInflate } from 'zlib';
import { createBrotliCompress, createBrotliDecompress, createDeflate, createGunzip, createGzip, createInflate } from 'zlib';

/**
* Available [content encoders](https://github.com/hapijs/hapi/blob/master/API.md#-serverencoderencoding-encoder).
Expand All @@ -7,6 +7,7 @@ export interface ContentEncoders {

deflate: typeof createDeflate;
gzip: typeof createGzip;
br: typeof createBrotliCompress;
}

/**
Expand All @@ -16,4 +17,5 @@ export interface ContentDecoders {

deflate: typeof createInflate;
gzip: typeof createGunzip;
br: typeof createBrotliDecompress;
}
24 changes: 24 additions & 0 deletions test/payload.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,30 @@ describe('Payload', () => {
expect(res.result).to.equal(message);
});

it('handles br payload', async () => {

const message = { 'msg': 'This message is going to be brotlied.' };
const server = Hapi.server();
server.route({ method: 'POST', path: '/', handler: (request) => request.payload });

const compressed = await new Promise((resolve) => Zlib.brotliCompress(JSON.stringify(message), (ignore, result) => resolve(result)));

const request = {
method: 'POST',
url: '/',
headers: {
'content-type': 'application/json',
'content-encoding': 'br',
'content-length': compressed.length
},
payload: compressed
};

const res = await server.inject(request);
expect(res.result).to.exist();
expect(res.result).to.equal(message);
});

it('handles custom compression', async () => {

const message = { 'msg': 'This message is going to be gzipped.' };
Expand Down
70 changes: 62 additions & 8 deletions test/transmit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -677,6 +677,19 @@ describe('transmission', () => {
expect(res3.headers['last-modified']).to.equal(res2.headers['last-modified']);
});

it('returns a brotlied file in the response when the request accepts br', async () => {

const server = Hapi.server({ compression: { minBytes: 1 }, routes: { files: { relativeTo: __dirname } } });
await server.register(Inert);
server.route({ method: 'GET', path: '/file', handler: (request, h) => h.file(__dirname + '/../package.json') });

const res = await server.inject({ url: '/file', headers: { 'accept-encoding': 'br' } });
expect(res.headers['content-type']).to.equal('application/json; charset=utf-8');
expect(res.headers['content-encoding']).to.equal('br');
expect(res.headers['content-length']).to.not.exist();
expect(res.payload).to.exist();
});

it('returns a gzipped file in the response when the request accepts gzip', async () => {

const server = Hapi.server({ compression: { minBytes: 1 }, routes: { files: { relativeTo: __dirname } } });
Expand Down Expand Up @@ -729,6 +742,16 @@ describe('transmission', () => {
expect(res.payload).to.exist();
});

it('returns a brotlied stream response without a content-length header when accept-encoding is br', async () => {

const server = Hapi.server({ compression: { minBytes: 1 } });
server.route({ method: 'GET', path: '/stream', handler: () => new internals.TimerStream() });

const res = await server.inject({ url: '/stream', headers: { 'Content-Type': 'application/json', 'accept-encoding': 'br' } });
expect(res.statusCode).to.equal(200);
expect(res.headers['content-length']).to.not.exist();
});

it('returns a gzipped stream response without a content-length header when accept-encoding is gzip', async () => {

const server = Hapi.server({ compression: { minBytes: 1 } });
Expand All @@ -749,6 +772,37 @@ describe('transmission', () => {
expect(res.headers['content-length']).to.not.exist();
});

it('returns a br response on a post request when accept-encoding: br is requested', async () => {

const data = '{"test":"true"}';

const server = Hapi.server({ compression: { minBytes: 1 } });
server.route({ method: 'POST', path: '/', handler: (request) => request.payload });
await server.start();

const uri = 'http://localhost:' + server.info.port;
const brotlied = await internals.compress('brotliCompress', Buffer.from(data));

const { payload } = await Wreck.post(uri, { headers: { 'accept-encoding': 'br' }, payload: data });
expect(payload.toString()).to.equal(brotlied.toString());
await server.stop();
});

it('returns a br response on a get request when accept-encoding: br is requested', async () => {

const data = '{"test":"true"}';

const server = Hapi.server({ compression: { minBytes: 1 } });
server.route({ method: 'GET', path: '/', handler: () => data });
await server.start();

const uri = 'http://localhost:' + server.info.port;
const brotlied = await internals.compress('brotliCompress', Buffer.from(data));
const { payload } = await Wreck.get(uri, { headers: { 'accept-encoding': 'br' } });
expect(payload.toString()).to.equal(brotlied.toString());
await server.stop();
});

it('returns a gzip response on a post request when accept-encoding: gzip is requested', async () => {

const data = '{"test":"true"}';
Expand Down Expand Up @@ -835,7 +889,7 @@ describe('transmission', () => {
await server.stop();
});

it('returns a gzip response on a post request when accept-encoding: gzip;q=1, deflate;q=0.5 is requested', async () => {
it('returns a gzip response on a post request when accept-encoding: gzip;q=1, deflate;q=0.5, br;q=0.7 is requested', async () => {

const data = '{"test":"true"}';
const server = Hapi.server({ compression: { minBytes: 1 } });
Expand All @@ -844,12 +898,12 @@ describe('transmission', () => {

const uri = 'http://localhost:' + server.info.port;
const zipped = await internals.compress('gzip', Buffer.from(data));
const { payload } = await Wreck.post(uri, { headers: { 'accept-encoding': 'gzip;q=1, deflate;q=0.5' }, payload: data });
const { payload } = await Wreck.post(uri, { headers: { 'accept-encoding': 'gzip;q=1, deflate;q=0.5, br;q=0.7' }, payload: data });
expect(payload.toString()).to.equal(zipped.toString());
await server.stop();
});

it('returns a gzip response on a get request when accept-encoding: gzip;q=1, deflate;q=0.5 is requested', async () => {
it('returns a gzip response on a get request when accept-encoding: gzip;q=1, deflate;q=0.5, br;q=0.7 is requested', async () => {

const data = '{"test":"true"}';
const server = Hapi.server({ compression: { minBytes: 1 } });
Expand All @@ -858,12 +912,12 @@ describe('transmission', () => {

const uri = 'http://localhost:' + server.info.port;
const zipped = await internals.compress('gzip', Buffer.from(data));
const { payload } = await Wreck.get(uri, { headers: { 'accept-encoding': 'gzip;q=1, deflate;q=0.5' } });
const { payload } = await Wreck.get(uri, { headers: { 'accept-encoding': 'gzip;q=1, deflate;q=0.5, br;q=0.7' } });
expect(payload.toString()).to.equal(zipped.toString());
await server.stop();
});

it('returns a deflate response on a post request when accept-encoding: deflate;q=1, gzip;q=0.5 is requested', async () => {
it('returns a deflate response on a post request when accept-encoding: deflate;q=1, gzip;q=0.5, br;q=0.7 is requested', async () => {

const data = '{"test":"true"}';
const server = Hapi.server({ compression: { minBytes: 1 } });
Expand All @@ -872,12 +926,12 @@ describe('transmission', () => {

const uri = 'http://localhost:' + server.info.port;
const deflated = await internals.compress('deflate', Buffer.from(data));
const { payload } = await Wreck.post(uri, { headers: { 'accept-encoding': 'deflate;q=1, gzip;q=0.5' }, payload: data });
const { payload } = await Wreck.post(uri, { headers: { 'accept-encoding': 'deflate;q=1, gzip;q=0.5, br;q=0.7' }, payload: data });
expect(payload.toString()).to.equal(deflated.toString());
await server.stop();
});

it('returns a deflate response on a get request when accept-encoding: deflate;q=1, gzip;q=0.5 is requested', async () => {
it('returns a deflate response on a get request when accept-encoding: deflate;q=1, gzip;q=0.5, br;q=0.7 is requested', async () => {

const data = '{"test":"true"}';
const server = Hapi.server({ compression: { minBytes: 1 } });
Expand All @@ -886,7 +940,7 @@ describe('transmission', () => {

const uri = 'http://localhost:' + server.info.port;
const deflated = await internals.compress('deflate', Buffer.from(data));
const { payload } = await Wreck.get(uri, { headers: { 'accept-encoding': 'deflate;q=1, gzip;q=0.5' } });
const { payload } = await Wreck.get(uri, { headers: { 'accept-encoding': 'deflate;q=1, gzip;q=0.5, br;q=0.7' } });
expect(payload.toString()).to.equal(deflated.toString());
await server.stop();
});
Expand Down