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

feat: handle number and object responses correctly in functions #440

Merged
merged 7 commits into from
Jul 22, 2022

Conversation

orinokai
Copy link
Contributor

@orinokai orinokai commented Jul 14, 2022

Summary

We currently handle Gatsby's function response helpers (basiclaly Express's send and json) by adding our own implementations to the mocked HTTP ServerResponse object that is sent back from the function lambda.

However, our implementations don't handle some types in the same way as Express and this PR addresses that.

Specifically:

  • objects sent using send are now stringified unless they are Buffers (which are now served with the correct content type)
  • numbers sent using send are used as the status code and the relevant status message is sent as the body

Test plan

Tests are in progress

Relevant links (GitHub issues, Notion docs, etc.) or a picture of cute animal

hetrochromatic cat licking its nose

@netlify
Copy link

netlify bot commented Jul 14, 2022

Deploy Preview for netlify-plugin-gatsby-demo ready!

Name Link
🔨 Latest commit 9ac9098
🔍 Latest deploy log https://app.netlify.com/sites/netlify-plugin-gatsby-demo/deploys/62da63f5ffe41a00095fc5fe
😎 Deploy Preview https://deploy-preview-440--netlify-plugin-gatsby-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@orinokai orinokai changed the title feat: handle number, boolean and object types with res.send feat: handle number and object responses correctly in functions Jul 14, 2022
@orinokai orinokai self-assigned this Jul 14, 2022
@orinokai orinokai force-pushed the rs/function-res-content-types branch 2 times, most recently from c7ce05c to 43e869a Compare July 18, 2022 10:17
package.json Outdated
@@ -38,7 +38,7 @@
},
"devDependencies": {
"@netlify/build": "^27.4.0",
"@netlify/eslint-config-node": "6.0.0",
"@netlify/eslint-config-node": "5.1.8",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

downgraded to avoid lint errors with module resolution after syncing package.json and package-lock.json

Copy link
Contributor

Choose a reason for hiding this comment

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

Odd that this is happening 🤔 Is it the rules within this project that are causing these issues?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are the errors related to the files you've changed, or elsewhere? If they're not related to your changes then it seems like this shouldbe a different PR: i.e. you do a separate PR to update the package-lock and if that breaks the linting, make the linting changes in there rather than in this unrelated PR

@@ -71,7 +72,7 @@
"npm-run-all": "^4.1.5",
"rimraf": "^3.0.2",
"tmp-promise": "^3.0.3",
"typescript": "^4.5.2"
"typescript": "4.6.4"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

upgraded to avoid ts.resolveTypeReferenceDirective error after syncing package.json and package-lock.json

Copy link
Contributor

Choose a reason for hiding this comment

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

[dust] Should this be the following instead?

Suggested change
"typescript": "4.6.4"
"typescript": "^4.6.4"

Or do we want to start pinning to specific versions to prevent issues going forward during package upgrades?

@@ -133,7 +134,7 @@ export const createResponseObject = ({ onResEnd }) => {
response.statusCode = statusCode
},
})
res.headers = { 'content-type': 'text/plain; charset=utf-8' }
res.headers = { 'content-type': 'text/html; charset=utf-8' }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

default content type changed to html to match gatsby serve

@orinokai orinokai marked this pull request as ready for review July 19, 2022 10:08
@orinokai orinokai requested a review from a team July 19, 2022 10:08
ericapisani
ericapisani previously approved these changes Jul 19, 2022
Copy link
Contributor

@ericapisani ericapisani left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

package.json Outdated
@@ -38,7 +38,7 @@
},
"devDependencies": {
"@netlify/build": "^27.4.0",
"@netlify/eslint-config-node": "6.0.0",
"@netlify/eslint-config-node": "5.1.8",
Copy link
Contributor

Choose a reason for hiding this comment

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

Odd that this is happening 🤔 Is it the rules within this project that are causing these issues?

@@ -71,7 +72,7 @@
"npm-run-all": "^4.1.5",
"rimraf": "^3.0.2",
"tmp-promise": "^3.0.3",
"typescript": "^4.5.2"
"typescript": "4.6.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

[dust] Should this be the following instead?

Suggested change
"typescript": "4.6.4"
"typescript": "^4.6.4"

Or do we want to start pinning to specific versions to prevent issues going forward during package upgrades?

@orinokai orinokai force-pushed the rs/function-res-content-types branch from 2e35993 to 9ac9098 Compare July 22, 2022 08:46
@kodiakhq kodiakhq bot merged commit 2335f5e into main Jul 22, 2022
@kodiakhq kodiakhq bot deleted the rs/function-res-content-types branch July 22, 2022 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants