pillarjs
ci: fix errors in ci github action for node 8 and 9
This PR fixes nyc version to 14.1.1 when running tests in node 8 or node 9. nyc 15.x requires a yargs package version that requires node >=10.
I've also added the latest versions of node (18, 19 and 20) to the matrix.
Related to https://github.com/jshttp/http-errors/issues/108
[!WARNING] The CI is failing for one test in node@21 and node@22. The error has nothing to do with this PR, because those node versions were not tested before and with this PR we discovered it.
The CI is failing for one test in node@21 and node@22. The error has nothing to do with this PR, because those node versions were not tested before and with this PR we discovered it.
Interesting, I don't have time to look into the failure right now, but the test is pretty simple and clear so likely this is a change in node as I don't think this package ever returns a 400, so likely we either need to update/remove this test.
@UlisesGascon , @wesleytodd
How should we proceed with this PR? Can we merge it and then fix the failing test in a separate PR? Or do you want to proceed differently?
So I just pulled this down and am poking around at it, and it is because node is sending back the 400 before it gets passed onto finalhandler. I was looking for a clear commit which would cause it but could not find it, but my guess is maybe the llhttp update? @ShogunPanda, would you mind taking a look to see if this was an expected breaking change?
I realized the thread here didn't have enough context, sorry about that, here is what I was hoping you could look at @shogunpanda:
The test passes an invalid path of /foo%20§. In node versions prior to 21 this was passed along to finalhandler and was not considered a 400 from node. Now it is returning a 400 with an empty body which circumvents finalhandler's 404 and html response.
@wesleytodd Yes, the llhttp is the "problem". Between 8 and 9 llhttp became strict by default: invalid characters in the URL are not supported anymore and return in a parse error.
Awesome, thanks for the direct confirmation. Now that we are sure that is the cause, I think we can just remove these tests for invalid paths since we will also be dropping node support prior to this soon and I dont think we have any other changes slated for this package which warrant trying to conditionally fix the test.
@inigomarquinez do you want to do that in this PR to get it passing before we merge? Or should we do it in a separate one? I am not super opinionated either way.