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(runtime/js/http): Correctly parse user response headers #10076

Merged
merged 2 commits into from
Apr 12, 2021

Conversation

nayeemrmn
Copy link
Collaborator

We were using a for ... in loop on a Headers object.

@lucacasonato
Copy link
Member

Just realized we also have no test for non 200 status code 🙃.

@nayeemrmn nayeemrmn force-pushed the http-response-headers branch 3 times, most recently from e50d52f to 1e56c13 Compare April 10, 2021 01:05
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines -107 to -112
function respond(responseSenderRid, resp, zeroCopyBuf) {
return Deno.core.jsonOpSync("op_http_response", [
responseSenderRid,
resp.status ?? 200,
flatEntries(resp.headers ?? {}),
], zeroCopyBuf);
Copy link
Member

Choose a reason for hiding this comment

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

Glad you removed this function, it was superfluous 👍

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

Is it possible to add a test to cli/tests/unit/http_test.ts? Not immediately obvious to me what this is fixing.

@nayeemrmn
Copy link
Collaborator Author

nayeemrmn commented Apr 12, 2021

@ry The following assertion fails on main because no headers from the user's Response are passed to op_http_response:

assertEquals(resp.headers.get("foo"), "bar");
Shoehorning it into the basic test seems fair for response headers.

@ry
Copy link
Member

ry commented Apr 12, 2021

Nayeem - sure that works

@ry ry merged commit a205046 into denoland:main Apr 12, 2021
@nayeemrmn nayeemrmn deleted the http-response-headers branch April 12, 2021 14:33
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.

None yet

4 participants