fix: call underlying implementation properly #183
Conversation
|
@dougwilson I just added a test. |
|
It is 3:30am my time, so I am headed off now. I'll be back tomorrow if I have time. Thanks for your hard work! |
|
@dougwilson do u think we can merge of this fix or there are something missing? |
|
Hi @fernandolguevara 👋 . Yea, I replied about the callback and also there is the comment regarding the tests that are still missing. |
|
Looks like some tests is failed. @fernandolguevara can you finish this PR? Seems very important for SvelteKit. |
|
@dougwilson can you run the checks again? now seems to be working on all defined versions |
|
@dougwilson how do you want to proceed with the merge of this change? |
|
Hey! So I ran the gh actions as you asked, and I was thinking you were going to fix the failure listed there. If not, let me know and I can jump in and help out. Ideally we'd want a green CI before merging, and ideally also a code review, which I haven't done just yet as I wasn't sure if you were done with making changes. |
|
@dougwilson I just added some tests to increase the codecoverage, there are only 3 lines left with specific code for node 0.8, can you help me with those? |
|
@dougwilson ping |
fernandolguevara
requested review from
7antra and
stalkerg
and removed request for
dougwilson and
7antra
|
Hi @fernandolguevara thanks for your ping. I will circle back on this asap once I finish working with a security vulnerability report on a diff module 👍 Sorry it got lost. If you could ping me again next week, that will help make sure it is lot lost again in the sea of notifications. |
|
Also not sure why this PR our github actions workflow will not run on. You may need to reopen a new pr to see if that fixes it. Or maybe a new push will kick it off. There is no option for me to even run manually. |
| 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({}) |
There was a problem hiding this comment.
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. 🤔
There was a problem hiding this comment.
we don't have access to the node internal errors... newer runtime versions add new ones, that's the main reason for this hack
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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?
|
@fernandolguevara if I dropped 0.8 for this, would thay simplify the code and hacks? Or even anything less than Node.js 4? |
|
here u can check CI Results https://github.com/fernandolguevara/compression/actions/runs/4666297771 |
|
Thank you , the commit push is indeed whay it needed to kick off. What are your thoughts on my question above? |
|
@dougwilson think the best is to push out this fix... then u can take the time needed for the big changes on the lib |
|
I'm not suggesting any big changes? Just trying to figure out how to not have that ServerResponse constructor and manipulation. I was hoping that maybe just dropping some Node.js versions would be enough? If not, we need the Node.js project to be aware so they don't break this module, because this is definitely way outside their API. I just cannot land this with a hack like that, so I am trying to work with you to figure out a way not to have it or get blessed from the Node.js project. |
| 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({}) |
There was a problem hiding this comment.
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?
chore(ci):added new node versions
related issue #5431