new way to pass the configuration to the encodings #232
Conversation
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
This PR proposes a reorganized and enhanced way to pass configuration to the compression encodings, enabling the ability to disable specific encodings.
- Introduces an "encodings" configuration object in the server setup.
- Adds new tests covering scenarios when various encodings (gzip, deflate, br) are disabled.
- Updates the compression implementation in index.js to use the new configuration.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/compression.js | New tests added to verify behavior when specific encodings are disabled, with some spelling corrections needed. |
| index.js | Updated configuration handling to pass encoding options and remove support for identity. |
Comments suppressed due to low confidence (1)
index.js:39
- The variable name 'ENCONDING_OPTIONS' contains a typo. Consider renaming it to 'ENCODING_OPTIONS' for clarity and consistency.
var ENCONDING_OPTIONS = ['br', 'gzip', 'deflate']
| @@ -51,6 +51,20 @@ describe('compression()', function () { | |||
| .expect(200, 'hello, world', done) | |||
| }) | |||
|
|
|||
| it('should skip if content-encondig is identity', function (done) { | |||
There was a problem hiding this comment.
There's a spelling error in the test description ('content-encondig'). Consider changing it to 'Content-Encoding' for clarity.
| it('should skip if content-encondig is identity', function (done) { | |
| it('should skip if Content-Encoding is identity', function (done) { |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| var server = createServer({ threshold: 0 }, function (req, res) { | ||
| res.setHeader('Content-Type', 'text/plain') | ||
| res.end('hello, world') | ||
| describe('encondigs', function () { |
There was a problem hiding this comment.
There's a spelling mistake in the description ('encondigs'). It should be corrected to 'encodings'.
| describe('encondigs', function () { | |
| describe('encodings', function () { |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| res.end('hello, world') | ||
| describe('encondigs', function () { | ||
| describe('when "Accept-Encoding: gzip"', function () { | ||
| it('when gzip is disable', function (done) { |
There was a problem hiding this comment.
The test description uses 'disable' instead of 'disabled'. Updating it to 'when gzip is disabled' would improve clarity.
| it('when gzip is disable', function (done) { | |
| it('when gzip is disabled', function (done) { |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| res.end('hello, world') | ||
|
|
||
| describe('when "Accept-Encoding: gzip, br"', function () { | ||
| it('should respond with gzip when brotli is disable', function (done) { |
There was a problem hiding this comment.
The test description uses 'disable' instead of 'disabled'. Consider updating it to 'should respond with gzip when brotli is disabled'.
| it('should respond with gzip when brotli is disable', function (done) { | |
| it('should respond with gzip when brotli is disabled', function (done) { |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
| @@ -36,11 +36,9 @@ module.exports.filter = shouldCompress | |||
| * @private | |||
| */ | |||
| var cacheControlNoTransformRegExp = /(?:^|,)\s*?no-transform\s*?(?:,|$)/ | |||
| var SUPPORTED_ENCODING = ['br', 'gzip', 'deflate', 'identity'] | |||
| var ENCONDING_OPTIONS = ['br', 'gzip', 'deflate'] | |||
There was a problem hiding this comment.
| var ENCONDING_OPTIONS = ['br', 'gzip', 'deflate'] | |
| var ENCODING_OPTIONS = ['br', 'gzip', 'deflate'] |
This is the new way I’m proposing to pass configuration to the encodings, which now does it in a more organized manner. Any suggestions are very welcome.
Adds the ability to disable supported encodings.
TODOS:
Closes #220
Closes #25
Closes #27