fix: ignore status message for HTTP/2 by alexander-akait · Pull Request #53 · pillarjs/finalhandler

alexander-akait · 2024-08-19T14:45:52Z

fixes#45
fixes#34

alexander-akait · 2024-08-19T14:46:20Z

+const servers = [
+ ['http', createHTTPServer],
+ ['http2', createHTTP2Server]
+]


Let's make full testing

No one test was deleted, just remove describe('finalhandler(req, res)', () => {}) wrapper to avoid extra nesting (reading purposes)

alexander-akait · 2024-08-19T16:45:27Z

Looks like we need ignore one case - https://github.com/pillarjs/finalhandler/actions/runs/10457238346/job/28956872842?pr=53, WIP 😄

wesleytodd · 2024-08-19T16:57:58Z

+ ['http2', createHTTP2Server]
+]
+
+for (var i = 0; i < servers.length; i++) {


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)); }

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.

alexander-akait · 2024-08-20T12:09:33Z

@wesleytodd Done, diff looks better

wesleytodd · 2024-08-20T14:31:56Z

+}
+
+function createHTTP2Server (err, opts) {
 return http.createServer(function (req, res) {


Am I missing something here? Is this supposed to be using http2? Sorry if I am being silly here, barely started my morning coffee.

alexander-akait · 2024-08-21T11:29:10Z

 }

-function createServer (err, opts) {
+function createHTTPServer (err, opts) {


Just rename

alexander-akait · 2024-08-21T11:29:25Z

 }
 }

+function rawrequestHTTP2 (server) {


For testing raw HTTP2 request

alexander-akait · 2024-08-21T11:32:29Z

+var nodeVersion = process.versions.node.split('.').map(Number)
+
+// `superagent` only supports `http2` since Node.js@10
+if (http2 && nodeVersion[0] >= 10) {


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

alexander-akait · 2024-08-21T11:41:24Z

+ }
+
+ return req
+ }


Wrapper for http2 because of https://github.com/ladjs/supertest/blob/master/test/supertest.js#L1403

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.

alexander-akait · 2024-08-21T11:42:16Z

+ // http2 does not support status message
+ var describeStatusMessage = !/statusMessage/.test(http.IncomingMessage.toString()) || type === 'http2'
+ ? describe.skip
+ : describe


Skip tests for statusMessage, because it is not supported for http2

alexander-akait · 2024-08-21T11:43:28Z

 })

 it('should override with err.status', function (done) {
- var server = http.createServer(function (req, res) {


Use createServer instead http.createServer to test using http2 too, the same for tests below

alexander-akait · 2024-08-21T11:44:28Z

+ })
+ })
+ })
+}


Let's make these test for http and http2

alexander-akait · 2024-08-21T11:45:18Z

/cc @wesleytodd Rewrote and corrected everything, everything works locally, plus left comments about the changes

wesleytodd · 2024-08-21T15:14:50Z

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.

alexander-akait · 2024-08-21T15:23:21Z

@wesleytodd

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 http2 on this version and that's it

alexander-akait · 2024-08-21T15:27:07Z

@wesleytodd it should work now, it's strange that using nvm locally the tests passed, fixes syntax

wesleytodd · 2024-08-21T15:50:48Z

I didn't quite understand you, we don't test only on them because it's not supported http2 on this version and that's it

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.

alexander-akait · 2024-08-21T15:54:03Z

@wesleytodd

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 nvm to test locally and all works fine, apparently nvm launches node.js with flags which enable support new features for old versions

wesleytodd

@pillarjs/express-tc Would love it if someone else could get some eyes on this. I think it looks ready to go.

alexander-akait · 2024-08-22T16:06:03Z

I think @pillarjs/express-tc doesn't work

wesleytodd · 2024-08-22T18:40:32Z

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.

wesleytodd · 2024-08-31T18:40:25Z

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

alexander-akait · 2024-09-03T11:42:04Z

@wesleytodd We need it for v1 too, due - https://github.com/senchalabs/connect and https://github.com/senchalabs/connect/blob/master/package.json#L21

alexander-akait · 2024-09-03T11:44:04Z

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

wesleytodd · 2024-09-03T14:11:44Z

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.

alexander-akait · 2024-09-03T14:23:29Z

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 1 version, we haven't changed anything critical, sorry, but the argument about deprecation old Node.js versions has no meaning here, it will take a long time before everything can be updated, also I don't know your roadmap and it was ready, and the current problem that I'm trying to solve is quite obvious and exists right now

wesleytodd · 2024-09-03T15:52:43Z

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.

but the argument about deprecation old Node.js versions has no meaning here

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.

fix: ignore status message for HTTP/2
9382f71

alexander-akait commented Aug 19, 2024
View reviewed changes

alexander-akait mentioned this pull request Aug 19, 2024
Ignore status message for HTTP/2 #45
Closed

wesleytodd requested changes Aug 19, 2024
View reviewed changes

Comment threadtest/test.js Outdated

test: fix
da59021

test: fix node@9
bd8c04a

wesleytodd reviewed Aug 19, 2024
View reviewed changes

refactor: tests
7795a8c

wesleytodd reviewed Aug 20, 2024
View reviewed changes

test: fix
e2f9f7f

alexander-akait commented Aug 21, 2024
View reviewed changes

Comment threadtest/support/utils.js
}
}

function rawrequestHTTP2 (server) {
alexander-akaitAug 21, 2024
Copy link
Copy Markdown
ContributorAuthor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For testing raw HTTP2 request

alexander-akait commented Aug 21, 2024
View reviewed changes

Comment threadtest/test.js
})
})
})
}
alexander-akaitAug 21, 2024
Copy link
Copy Markdown
ContributorAuthor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make these test for http and http2

test: fix syntax
8fcfd80

wesleytodd approved these changes Aug 21, 2024
View reviewed changes

wesleytodd changed the base branch from master to 2.xAugust 31, 2024 18:38

wesleytodd merged commit 6dd0318 into pillarjs:2.xAug 31, 2024

wesleytodd pushed a commit that referenced this pull request Sep 2, 2024
fix: ignore status message for HTTP/2 (#53)
1e3f2ae
* fix: ignore status message for HTTP/2 * test: fix * test: fix node@9 * refactor: tests * test: fix * test: fix syntax

alexander-akait deleted the fix-HTTP2 branch September 3, 2024 11:41

wesleytodd pushed a commit that referenced this pull request Sep 3, 2024
fix: ignore status message for HTTP/2 (#53)
28634e2

wesleytodd mentioned this pull request Sep 3, 2024
v1.3.0 Staging #58
Merged

wesleytodd pushed a commit that referenced this pull request Sep 3, 2024
fix: ignore status message for HTTP/2 (#53)
b30e165

wesleytodd pushed a commit that referenced this pull request Sep 3, 2024
fix: ignore status message for HTTP/2 (#53)
3b83a61

alexander-akait mentioned this pull request Sep 3, 2024
chore: finalhandler senchalabs/connect#1141
Open

bjohansebas mentioned this pull request Dec 8, 2024
support http2 expressjs/compression#211
Draft

zachleat mentioned this pull request Jul 25, 2025
Upgrade finalhandler dep from v1 to v2 11ty/dev-server#120
Closed

Uh oh!

Uh oh!

Conversation

alexander-akait commented Aug 19, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akaitAug 19, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexander-akait commented Aug 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait commented Aug 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait commented Aug 21, 2024

Uh oh!

wesleytodd commented Aug 21, 2024

Uh oh!

alexander-akait commented Aug 21, 2024

Uh oh!

alexander-akait commented Aug 21, 2024

Uh oh!

wesleytodd commented Aug 21, 2024

Uh oh!

alexander-akait commented Aug 21, 2024

Uh oh!

wesleytodd left a comment

Choose a reason for hiding this comment

Uh oh!

alexander-akait commented Aug 22, 2024

Uh oh!

wesleytodd commented Aug 22, 2024

Uh oh!

wesleytodd commented Aug 31, 2024

Uh oh!

alexander-akait commented Sep 3, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexander-akait commented Sep 3, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wesleytodd commented Sep 3, 2024

Uh oh!

alexander-akait commented Sep 3, 2024

Uh oh!

wesleytodd commented Sep 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alexander-akait commented Aug 19, 2024•
edited
Loading

alexander-akaitAug 19, 2024•
edited
Loading

alexander-akait commented Sep 3, 2024•
edited
Loading

alexander-akait commented Sep 3, 2024•
edited
Loading