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: concat body in res.end #27

Merged
merged 1 commit into from
Jan 26, 2022
Merged

Conversation

ErisDS
Copy link
Collaborator

@ErisDS ErisDS commented Jan 25, 2022

I'm not familiar with flow and am unable to get the tests to run locally as I just get a bunch of "Cannot resolve module lodash" style errors.

However I believe this will demonstrate the issue and fix it!

I have the fix I outlined in #26 working locally.


fixes: #26

  • added a test to demonstrate how write and end are used together that should fail with the current code
  • use the already bound write method to ensure that body is concatenated correctly
  • this seemed like the quickest way to do this, as callback isn't used

fixes: antongolub#26

- added a test to demonstrate how write and end are used together that should fail with the current code
- use the already bound write method to ensure that body is concatenated correctly
- this seemed like the quickest way to do this, as callback isn't used
@antongolub
Copy link
Owner

antongolub commented Jan 25, 2022

Hey, @ErisDS,

Nice catch, thanks a lot.

lgtm, but now some tests are broken, let's try to fix: I suggest using this.write instead of write.

    this.end = (chunk: IData, encoding?: ?string): IResponse => {
      this.write(chunk, encoding)
      this.emit('finish')
      return this
    }

Cannot resolve module lodash

Flow requires manual setup for external libdefs from the special registry (it's smth like DT for TS) via CLI: flow-typed install express@4.x.x lodash@4.x.x setprototypeof@1.x.x. They cannot simply be declared as package dev deps. Using post-install is also a bad option for obvious reasons.

@antongolub antongolub merged commit 5f78461 into antongolub:master Jan 26, 2022
@antongolub
Copy link
Owner

🎉 This PR is included in version 1.6.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ErisDS
Copy link
Collaborator Author

ErisDS commented Jan 27, 2022

I'm sorry I didn't manage to come back to this yesterday, I was trying to wrap up the tool I'm building so I could be sure this worked.

I have found another, different issue which I'll raise and also try to see if I can fix these tests.

Thanks for merging in the meantime, do you think the broken tests are actually wrong or that they're just not that important?

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

Successfully merging this pull request may close these issues.

.end overwrites res.body with the last character
2 participants