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

Changed String.proptype.length to Buffer.byteLength(result) #10046

Closed
wants to merge 1 commit into from
Closed

Changed String.proptype.length to Buffer.byteLength(result) #10046

wants to merge 1 commit into from

Conversation

dnguneratne
Copy link
Contributor

@dnguneratne dnguneratne commented Oct 23, 2018

Closes #10041

I found two places where String.prototype.length is used to calculate Content-Length.
The files are,
\current\core\server\api\shared\headers.js
versions\2.3.0\core\server\api\v0.1\index.js.

I switched JSON.stringify(result).length to Buffer.byteLength(result).

The test results were the same as before and after the change with 4 failing tests:
image

@dnguneratne dnguneratne changed the title Closes #10041: Changed String.proptype.length to Buffer.byteLength(result) Changed String.proptype.length to Buffer.byteLength(result) Oct 23, 2018
@naz naz self-assigned this Oct 23, 2018
@allouis
Copy link
Contributor

allouis commented Oct 24, 2018

Hey @dnguneratne thanks for the PR 😄

I don't think these tests were failing before this change - I would imagine the failure is due to incorrect usage of Buffer.byteLength which expects a string or buffer passed to it, rather than an object ☺️

@naz
Copy link
Contributor

naz commented Oct 25, 2018

We have merged in #10055 so the initial issue has been resolved. Thanks for your input @dnguneratne!

@naz naz closed this Oct 25, 2018
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.

3 participants