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

Add Content-Length HTTP header for non-stream JSON responses. #213

Closed

Conversation

newkek
Copy link

@newkek newkek commented Oct 25, 2019

This helps clients decode more efficiently HTTP responses.

I have left out the stream responses because I think the header doesn't apply there.

Also I have covered only the happy path (successful response), it seems errors are implicitly supposed to return a Content-Length of 0.

@newkek
Copy link
Author

newkek commented Nov 14, 2019

Hi, is there any comments on this?

@newkek
Copy link
Author

newkek commented Nov 14, 2019

@oliemansm maybe?

@oliemansm
Copy link
Member

The problem is that this PR conflicts with #215 that results in a consideral performance improvement. Accepting this PR would undo those changes. I don't think this PR would lead to a better performance than the other PR, but you could do benchmarks and prove me wrong :)

@oliemansm
Copy link
Member

@newkek That other PR actually ended up breaking things. so I've reverted it. Will manually merge your changes to the new refactoring I'm working on.

@oliemansm oliemansm closed this Nov 15, 2019
@oliemansm oliemansm added this to the 9.0.0 milestone Nov 15, 2019
@newkek newkek deleted the add-content-length-header branch June 15, 2020 16:30
@newkek
Copy link
Author

newkek commented Jun 15, 2020

Wow I am coming back to this very late but thanks @oliemansm for taking care of this change.

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

Successfully merging this pull request may close these issues.

2 participants