Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

fix: call underlying implementation properly #183

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
fernandolguevara wants to merge 40 commits into expressjs:v2
base: v2
Choose a base branch
Loading
from fernandolguevara:master

Conversation

@fernandolguevara
Copy link

@fernandolguevara fernandolguevara commented Jul 10, 2022

related issue #5431

itssumitrai, gyurielf, Antonio-Bennett, and fernandolguevara reacted with hooray emoji
Copy link
Author

@dougwilson I just added a test.

Copy link
Contributor

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!
fernandolguevara, umwwwelt, and stalkerg reacted with thumbs up emoji umwwwelt and stalkerg reacted with heart emoji

Copy link
Author

@dougwilson do u think we can merge of this fix or there are something missing?

Copy link
Contributor

dougwilson commented Jul 11, 2022 •
edited
Loading

Hi @fernandolguevara 👋 . Yea, I replied about the callback and also there is the comment regarding the tests that are still missing.

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior of callback write after ended should match Node.js and tests are needed for all the added code paths. Easy way to know tests are missing is if you revert a part of the patch and tests still pass, that part was untested.

umwwwelt reacted with eyes emoji
Copy link

Looks like some tests is failed. @fernandolguevara can you finish this PR? Seems very important for SvelteKit.
fernandolguevara reacted with thumbs up emoji

Copy link
Contributor

@fernandolguevara if I dropped 0.8 for this, would thay simplify the code and hacks? Or even anything less than Node.js 4?

Copy link
Author

Copy link
Contributor

Thank you , the commit push is indeed whay it needed to kick off. What are your thoughts on my question above?

Copy link
Author

@dougwilson think the best is to push out this fix... then u can take the time needed for the big changes on the lib

Copy link
Contributor

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.

index.js Outdated
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({})
Copy link
Contributor

@dougwilson dougwilson Apr 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@bjohansebas bjohansebas changed the base branch from master to v2 April 16, 2025 03:43
Copy link
Member

Hi @fernandolguevara, support for old Node versions is no longer necessary, so we no longer need to use hacks to support those versions. I think this is going to be the PR where I clean up some of that old support, removing unnecessary things for Node 0.x compatibility. So I'll work with you to fix this small bug.

 

Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
index.js Outdated
Comment on lines 230 to 232
stream.on('error', function (err) {
res.emit('error', err)
})
Copy link
Member

@bjohansebas bjohansebas May 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part here causes the tests to not run correctly, for some reason, the request never finishes.

 

Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bjohansebas bjohansebas bjohansebas left review comments

@stalkerg stalkerg Awaiting requested review from stalkerg

+2 more reviewers

@dougwilson dougwilson dougwilson requested changes

@umwwwelt umwwwelt umwwwelt approved these changes

Reviewers whose approvals may not affect merge requirements

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /