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

fix: call underlying implementation properly #183

Open
wants to merge 32 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
f4a997e
fix: call underlying implementation properly
fernandolguevara Jul 10, 2022
3ad026f
test(response): test response write with cb
fernandolguevara Jul 10, 2022
03361ec
test(response.write): fix test expect
fernandolguevara Jul 10, 2022
d76b3cb
fix(response.write): call underlying implementation properly
fernandolguevara Jul 10, 2022
4653825
test(response.write): should have Content-Encoding header
fernandolguevara Jul 10, 2022
2440598
fix(response.write): handle optional arguments
fernandolguevara Jul 10, 2022
2991c82
fix(response.write): handle end(cb)
fernandolguevara Jul 10, 2022
2b3369c
refactor(response-proxy): match nodejs behavior
fernandolguevara Jul 12, 2022
3493e8c
test(compression): avoid arrow fn
fernandolguevara Jul 12, 2022
50ca417
test(compression): add ERR_STREAM_ALREADY_FINISHED
fernandolguevara Jul 12, 2022
e87ca6b
fix(package): add missing dep
fernandolguevara Jul 13, 2022
0ec6742
Merge branch 'master' into master
fernandolguevara Jul 13, 2022
48c7ad6
fix(package): format
fernandolguevara Jul 13, 2022
1abb056
fix(package): use sinon 4.5.0 target
fernandolguevara Jul 16, 2022
6d20bec
Merge branch 'master' of github.com:expressjs/compression
fernandolguevara Jul 23, 2022
ee0c68c
refactor(lib): remove assert-is-uint8array dependency
fernandolguevara Aug 6, 2022
b96d7ac
fix(package): remove semver
fernandolguevara Aug 6, 2022
0736a7f
fix(package): typo in test script
fernandolguevara Aug 6, 2022
27c391f
use sinon 3.3.0
fernandolguevara Aug 6, 2022
96bccad
remove sinon
fernandolguevara Aug 6, 2022
bd4ff2b
remove deconstruct
fernandolguevara Aug 6, 2022
50aa251
listen error event
fernandolguevara Aug 6, 2022
1ecf4da
fix res.write proxy
fernandolguevara Aug 6, 2022
088b23c
emit stream error on res
fernandolguevara Aug 6, 2022
3c69597
fix node 0.8/0.10/0.12 issues
fernandolguevara Aug 6, 2022
5ed9a90
fix lint error
fernandolguevara Aug 6, 2022
8d9fb6c
remove console.log
fernandolguevara Aug 6, 2022
17e6933
fix(test): add code coverage
fernandolguevara Aug 16, 2022
68ee980
fix: ask for runtime version once
fernandolguevara Apr 11, 2023
696e58f
chore(ci):added new node versions
fernandolguevara Mar 3, 2024
3a98b98
chore(ci): bring back node 8/9
fernandolguevara Mar 3, 2024
b89ebed
Merge pull request #1 from fernandolguevara/add-node-versions
fernandolguevara Mar 3, 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
90 changes: 80 additions & 10 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ var debug = require('debug')('compression')
var onHeaders = require('on-headers')
var vary = require('vary')
var zlib = require('zlib')
var ServerResponse = require('http').ServerResponse

/**
* Module exports.
Expand All @@ -36,6 +37,10 @@ module.exports.filter = shouldCompress
*/

var cacheControlNoTransformRegExp = /(?:^|,)\s*?no-transform\s*?(?:,|$)/
var hasUint8Array = (typeof Uint8Array === 'function')
function isUint8Array (arg) {
return hasUint8Array && arg && (arg instanceof Uint8Array || arg.toString() === '[object Uint8Array]')
}

/**
* Compress response data with gzip / deflate.
Expand All @@ -56,6 +61,8 @@ function compression (options) {
threshold = 1024
}

function noop () { }

return function compression (req, res, next) {
var ended = false
var length
Expand All @@ -75,23 +82,75 @@ function compression (options) {

// proxy

res.write = function write (chunk, encoding) {
if (ended) {
res.write = function write (chunk, encoding, callback) {
if (chunk === null) {
// throw ERR_STREAM_NULL_VALUES
return _write.call(this, chunk, encoding, callback)
} else if (typeof chunk === 'string' || typeof chunk.fill === 'function' || isUint8Array(chunk)) {
// noop
} else {
// throw ERR_INVALID_ARG_TYPE
return _write.call(this, chunk, encoding, callback)
}

if (!callback && typeof encoding === 'function') {
callback = encoding
encoding = undefined
}

if (typeof callback !== 'function') {
callback = noop
}

if (res.destroyed || res.finished || ended) {
// HACK: node doesn't expose internal errors,
// we need to fake response to throw underlying errors type
var fakeRes = new ServerResponse({})
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this solution is quite hacky and has a high likelihood of breaking in a future Node.js. we really should try to think on a better solution for this than creating a response object and trying to fake a socket object just to get an error. 🤔

Copy link
Author

@fernandolguevara fernandolguevara Apr 11, 2023

Choose a reason for hiding this comment

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

we don't have access to the node internal errors... newer runtime versions add new ones, that's the main reason for this hack

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I understand, but the Node.js project seems to be agressive is breaking projects who are "using the api wrong" even though citgm tells them it is broken, at lesst in the https components. Probably should check if this module it is citgm...

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this module is not included it citgm, so really cannot use a hack like this without some blessing from node.js... is this needed if we drop some node.js versions?

fakeRes.on('error', function (err) {
res.emit('error', err)
})
fakeRes.destroyed = res.destroyed
fakeRes.finished = res.finished || ended
// throw ERR_STREAM_DESTROYED or ERR_STREAM_WRITE_AFTER_END
_write.call(fakeRes, chunk, encoding, callback)
return false
fernandolguevara marked this conversation as resolved.
Show resolved Hide resolved
}

if (!this._header) {
this._implicitHeader()
}

if (chunk) {
chunk = toBuffer(chunk, encoding)
if (/^v0\.8\./.test(process.version) && stream) {
fernandolguevara marked this conversation as resolved.
Show resolved Hide resolved
encoding = callback
}
}

return stream
? stream.write(toBuffer(chunk, encoding))
: _write.call(this, chunk, encoding)
? stream.write(chunk, encoding, callback)
: _write.call(this, chunk, encoding, callback)
}

res.end = function end (chunk, encoding) {
if (ended) {
return false
res.end = function end (chunk, encoding, callback) {
if (!callback) {
if (typeof chunk === 'function') {
callback = chunk
chunk = encoding = undefined
} else if (typeof encoding === 'function') {
callback = encoding
encoding = undefined
}
}

if (typeof callback !== 'function') {
callback = noop
}

if (this.destroyed || this.finished || ended) {
this.finished = ended
// throw ERR_STREAM_WRITE_AFTER_END or ERR_STREAM_ALREADY_FINISHED
return _end.call(this, chunk, encoding, callback)
}

if (!this._header) {
Expand All @@ -104,16 +163,23 @@ function compression (options) {
}

if (!stream) {
return _end.call(this, chunk, encoding)
return _end.call(this, chunk, encoding, callback)
}

// mark ended
ended = true

if (chunk) {
chunk = toBuffer(chunk, encoding)
if (/^v0\.8\./.test(process.version) && stream) {
encoding = callback
}
}

// write Buffer for Node.js 0.8
return chunk
? stream.end(toBuffer(chunk, encoding))
: stream.end()
? stream.end(chunk, encoding, callback)
: stream.end(chunk, callback)
}

res.on = function on (type, listener) {
Expand Down Expand Up @@ -202,6 +268,10 @@ function compression (options) {
res.removeHeader('Content-Length')

// compression
stream.on('error', function (err) {
res.emit('error', err)
})

stream.on('data', function onStreamData (chunk) {
if (_write.call(res, chunk) === false) {
stream.pause()
Expand Down
Loading