Skip to content

fix(response): brings Response object to parity with Functions #287

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

Merged
merged 2 commits into from
Jun 29, 2021

Conversation

philnash
Copy link
Contributor

I was checking the code behind the Response object that Functions uses and noticed some differences, such as:

  • set/append functions returning this so that you can chain functions
  • body is initialized as null not undefined
  • being able to set headers, body and status code in the constructor

I think this brings the serverless toolkit version of the Response up to parity in both twilio-run and runtime-handler.

Contributing to Twilio

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.

test('sets status code', () => {
const response = new Response();
expect(response['statusCode']).toBe(200);
response.setStatusCode(418);
const response2 = response.setStatusCode(418);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the better test here be to re-check response because technically even though it returns itself we want to make sure it updated the original response object? This test would still succeed if we accidentally changed the logic where setStatusCode returns a new object

Copy link
Contributor

Choose a reason for hiding this comment

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

and then have a dedicated test that calling the same function twice chained returns the same instance.

This really applies throughout the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that this should have been a different test, that was lazy on my behalf 😄. I've updated that now.

I don't think we need to call this twice and test after though. Using expect(object1).toBe(object2) checks referential identity of the objects. So if we started returning a new object from setStatusCode, or the other functions here, this test would fail. Then the existing tests ensure that the changes are correctly made on the original object.

@dkundel dkundel merged commit 0c66d97 into main Jun 29, 2021
@dkundel dkundel deleted the response-parity branch June 29, 2021 16:13
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