fix: ignore status message for HTTP/2 #53
Conversation
test/test.js
Outdated
| const servers = [ | ||
| ['http', createHTTPServer], | ||
| ['http2', createHTTP2Server] | ||
| ] |
There was a problem hiding this comment.
Let's make full testing
There was a problem hiding this comment.
No one test was deleted, just remove describe('finalhandler(req, res)', () => {}) wrapper to avoid extra nesting (reading purposes)
|
Looks like we need ignore one case - https://github.com/pillarjs/finalhandler/actions/runs/10457238346/job/28956872842?pr=53, WIP 😄 |
test/test.js
Outdated
| ['http2', createHTTP2Server] | ||
| ] | ||
|
|
||
| for (var i = 0; i < servers.length; i++) { |
There was a problem hiding this comment.
I am not sure it matters, but one thing we could do to reduce the change set here would be to do the loop outside of the old code to not make it indent more. Something like this:
var topDescribe = function(createServer) { return function () { /* ... */} }
for (var i = 0; i < servers.length; i++) {
describe(name, topDescribe(createServer));
}
There was a problem hiding this comment.
To remove the indent you would need that inner function to be named, but just wanted to do some example code to show how to write this without the whole file being marked as changed.
|
@wesleytodd Done, diff looks better |
test/support/utils.js
Outdated
| }) | ||
| } | ||
|
|
||
| function createHTTP2Server (err, opts) { | ||
| return http.createServer(function (req, res) { |
There was a problem hiding this comment.
Am I missing something here? Is this supposed to be using http2? Sorry if I am being silly here, barely started my morning coffee.
| @@ -26,7 +37,7 @@ function createError (message, props) { | |||
| return err | |||
| } | |||
|
|
|||
| function createServer (err, opts) { | |||
| function createHTTPServer (err, opts) { | |||
There was a problem hiding this comment.
Just rename
| @@ -107,6 +131,72 @@ function rawrequest (server) { | |||
| } | |||
| } | |||
|
|
|||
| function rawrequestHTTP2 (server) { | |||
There was a problem hiding this comment.
For testing raw HTTP2 request
| var nodeVersion = process.versions.node.split('.').map(Number) | ||
|
|
||
| // `superagent` only supports `http2` since Node.js@10 | ||
| if (http2 && nodeVersion[0] >= 10) { |
There was a problem hiding this comment.
superagent (under the hood of supertest) supports http2 since Node.js@10, for old versions you will get:
Error: superagent: this version of Node.js does not support http2
| } | ||
|
|
||
| return req | ||
| } |
There was a problem hiding this comment.
Wrapper for http2 because of https://github.com/ladjs/supertest/blob/master/test/supertest.js#L1403
There was a problem hiding this comment.
I am wondering if we could have wrapper make the request call? Then if we just renamed it to request all the below code would remain unchanged. I am not going to consider this blocking, and I recognize you have put in a bunch of good work getting this here, so I hate asking for more for such nitpicks, but it would be nice to have the diff be more clear.
| // http2 does not support status message | ||
| var describeStatusMessage = !/statusMessage/.test(http.IncomingMessage.toString()) || type === 'http2' | ||
| ? describe.skip | ||
| : describe |
There was a problem hiding this comment.
Skip tests for statusMessage, because it is not supported for http2
| .expect(500, done) | ||
| }) | ||
|
|
||
| it('should override with err.status', function (done) { | ||
| var server = http.createServer(function (req, res) { |
There was a problem hiding this comment.
Use createServer instead http.createServer to test using http2 too, the same for tests below
| }) | ||
| }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Let's make these test for http and http2
|
/cc @wesleytodd Rewrote and corrected everything, everything works locally, plus left comments about the changes |
|
Sorry, we will be dropping these old node versions which dont support new language features. It will really help folks trying to contribute not have to deal with failures like this. Unfortunately we had a nearly 10 years of stalling out to catch up on here. Rest assured, we are working toward fixing this situation. |
I didn't quite understand you, we don't test only on them because it's not supported |
|
@wesleytodd it should work now, it's strange that using nvm locally the tests passed, fixes syntax |
Right, but the test failure was because you used an arrow function. So even though those tests were not meant to run in 0.8, you can't even use arrow functions anywhere in the code or it will fail. That is all I meant. |
Yeah, fixed, as I written above it is strange because I use |
|
I think |
|
It works, but apparently I never updated this membership list to the current Express TC. I will do that now, and will also post in our slack to help make sure folks see this. |
|
Sorry to switch this at the lats minute, but I am going to release 2.x right now so figured it is easier to do this in one release. Since you have done the things needed to land this in the 1.x line, if you need it there let me know and I can back port it as well, but on the chance you dont need it that badly I didn't want to do the extra release if we didn't need to. |
* fix: ignore status message for HTTP/2 * test: fix * test: fix node@9 * refactor: tests * test: fix * test: fix syntax
|
@wesleytodd We need it for v1 too, due - https://github.com/senchalabs/connect and https://github.com/senchalabs/connect/blob/master/package.json#L21 |
|
I want to avoid creation another conect compatibility framework, don't know why we need a major release, it is just a message with deprication |
|
We needed it because the entire Express project is dropping old node versions. If we are touching libraries we are doing majors to drop support, it had nothing to do with this change. We only have a few folks who have volunteered and shown up to work on this stuff, so I was trying to be efficient with my time. I can back port it to 1.x, might just be a bit while I get setup for the day. |
|
that would be good, we can't update it in 3rd modules, I am afraid you really will be overloaded issues from tens of thousands of developers, all I try to do is reduce your potential problems, I don't see any problem to use it for |
|
Yep thanks, I think with all the changes we have been pushing on these proejcts I forgot that you had said you needed it on 1.x. I should have just released it on both, my bad. Was a long weekend and I was spread way too thin.
Sorry if I was not clear, but I just meant I had to push a major for entirely unreleated reasons to this PR. Originally I was not even planning on releasing a 1.x release but then the issue with the null socket came up, and in my flurry of work I forgot to include backporting this to the 1.x release first. Please understand that these packages were left dormant for like 6 years, and we are just a small group of volunteers trying to get them back to a functional state. |
fixes #45
fixes #34
/cc @wesleytodd