Skip to content
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

fix: send Buffer with length #2232

Merged
merged 2 commits into from Oct 19, 2021
Merged

fix: send Buffer with length #2232

merged 2 commits into from Oct 19, 2021

Conversation

@Beretta1979
Copy link
Contributor

@Beretta1979 Beretta1979 commented Oct 18, 2021

#A buffer with length 0 should still be sent
See https://github.com/nodejs/node/blob/3f11666dc7e3a6d1221bde5145929dc72edc142e/lib/_http_outgoing.js#L788

fixes #2231

I wanted to target the 13.x branch, but that one was non existent ...

@Beretta1979
Copy link
Contributor Author

@Beretta1979 Beretta1979 commented Oct 18, 2021

The failing test (write callback is not called if the provided chunk is an empty buffer) could be fixed with changing:

const buf = Buffer.from('') req.write(null, null, reqWriteCallback)
to
req.write(null, null, reqWriteCallback)
If I look at the chainges which introduced this test it would make more sense (the description needs to change too)

Loading

@gr2m
Copy link
Member

@gr2m gr2m commented Oct 18, 2021

I wanted to target the 13.x branch, but that one was non existent ...

Everything merged into current main publishes to the current version, which is 13. We only have 12.x etc branches so we can backport fixes to previous versions

The failing test (write callback is not called if the provided chunk is an empty buffer) could be fixed with changing:

const buf = Buffer.from('') req.write(null, null, reqWriteCallback) to req.write(null, null, reqWriteCallback) If I look at the chainges which introduced this test it would make more sense (the description needs to change too)

can you push the change?

Loading

@Beretta1979
Copy link
Contributor Author

@Beretta1979 Beretta1979 commented Oct 19, 2021

I updated the unit test and also added the test which reproduces the issue

Loading

@gr2m
Copy link
Member

@gr2m gr2m commented Oct 19, 2021

@all-contributors please add @Beretta1979 for code and test

Loading

@allcontributors
Copy link
Contributor

@allcontributors allcontributors bot commented Oct 19, 2021

@gr2m

I've put up a pull request to add @Beretta1979! 🎉

Loading

gr2m
gr2m approved these changes Oct 19, 2021
Copy link
Member

@gr2m gr2m left a comment

great PR, thank you 💐 I confirmed that the your test failed before adding the fix

Loading

@gr2m gr2m changed the title fix(2231): A buffer with length 0 should still be sent fix: send Buffer with length Oct 19, 2021
@gr2m gr2m merged commit 8fcc607 into nock:main Oct 19, 2021
17 checks passed
Loading
@github-actions
Copy link

@github-actions github-actions bot commented Oct 19, 2021

🎉 This PR is included in version 13.1.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Loading

@Beretta1979
Copy link
Contributor Author

@Beretta1979 Beretta1979 commented Oct 19, 2021

Thanks! And thumbs up for this nice easy to use package!

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants