compression
compression copied to clipboard
expressjs
fix: call underlying implementation properly
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, do you know if I can run the CI locally? I'm using arm64, I don't know if the docker images will be compatible
Hi @fernandolguevara the CI just runs npm run test-ci command. You can run that locally.
Thanks for making this change, waiting eagerly for this change to roll out 🙏
@dougwilson that command works perfectly on my local environment, but seems to be an error with the github actions env, unfortunately I can't reproduce the CI error on my local env, I tried to build a docker container using the node 0.8 image but I can't run the test-ci command bc I got an error message when installing project dependencies.
can u help me to fix this issue?
it seems like a syntax issue (too old nodejs?) in third party library supports-color, I suppose nothing related to such a fix.
Do we really need to support some of these old node versions ? Before Node 14 all versions has been EOLed a while back https://endoflife.date/nodejs
Seems like a good opportunity to deprecate old Node versions and for a major version bump in this module. Since this basically runs in every single Express app, I'm sure they need to support super old versions of Node. But modernizing would be a good idea as more frameworks implement streaming responses, e.g. SvelteKit.
Yikes... some of the dependencies have high severity security issues. More cause for upgrades and modernization.
Hi @fernandolguevara I am not currently by a computer but didn't want you to think I didn't see your message. I went in gh actions and reran a bunch of them just to make sure they were succeeding outside of the pr. So yea, I'm not sure what change in this pr is causing that strange error, but I will look as soon as I can.
As for deprecating old Node.js versions, I think that is fine. Ideally if this change can work in them that would be ideal, at least to make a final release with this before a major and forcing folks to make a harder decision. I don't see any particular reason it shouldn't work for all the Node.js versions, but I haven't looked in to that yet. I see Node.js 10 shows a failure just on one of the tests added, and Node.js 4 a syntax error. So seems a multitude of errors. Has the change at least been validated in all the Node.js versions still in support by Node.js project, at least?
As for security issues in dependencies, we should not have any, but I am not at a computer to check, so I will have to take your word for it @coryvirok . I thought I had just updated a bunch of them a couple months ago, so I guess there are some new ones...
the error in node 10 occurs bc the runtime does not trigger the ERR_STREAM_DESTROYED error, should we skip this test?
https://github.com/nodejs/node/blob/v10.24.1/lib/_http_outgoing.js#L47
@dougwilson hello, sorry for interrupting but do you have any time frame for fixing it? Like this week or this month?
Hi @itssumitrai I don't think there has been any changes to the code since my last comment. Anyone is welcome to dig in and help the OP with changes to fix the issues with the build in this PR, not just me. I did volunteer to help as soon as I can, though. I am just away still since my comment and haven't gotten home to my computer to really run code and dig in to the issue, but I will as soon as I do.
I am really concerned with the findings that @coryvirok posted above, so I do plan to look into that before this, as of course fixing security issues is of the up-most importance.
@dougwilson, I did a check and here are my results. Hope this reduces your load.
| Commit | Vulnerabilities |
|---|---|
| https://github.com/expressjs/compression/commit/ad5113b98cafe1382a0ece30bb4673707ac59ce7 | 0 |
| https://github.com/fernandolguevara/compression/commit/6d20bec91fed1220a564df04f71ea9b5c243151e | 0 |
Hi, the build seems to be failing on Nodejs 0.8
npm ERR! TypeError: Object.keys called on non-object
npm ERR! at Function.keys (native)
npm ERR! at installTargetsError (/home/runner/.nvm/v0.8.28/lib/node_modules/npm/lib/cache.js:708:24)
npm ERR! at /home/runner/.nvm/v0.8.28/lib/node_modules/npm/lib/cache.js:641:10
npm ERR! at saved (/home/runner/.nvm/v0.8.28/lib/node_modules/npm/node_modules/npm-registry-client/lib/get.js:138:7)
npm ERR! at Object.oncomplete (fs.js:108:15)
npm ERR! If you need help, you may report this log at:
npm ERR! <http://github.com/isaacs/npm/issues>
npm ERR! or email it to:
npm ERR! <[email protected]>
Because its expected as assert-is-uint8array pkg added in this PR only supports 0.10 and above https://github.com/stdlib-js/assert-is-uint8array/blob/main/package.json#L59
@fernandolguevara maybe you should disable Nodejs 0.8 from the workflow if you want to use this pkg ?
@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?
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.
@fernandolguevara if I dropped 0.8 for this, would thay simplify the code and hacks? Or even anything less than Node.js 4?
@dougwilson
here u can check CI Results
https://github.com/fernandolguevara/compression/actions/runs/4666297771