Skip to content

fix: allow returning, number and boolean as well#65

Merged
pi0 merged 6 commits intomainfrom
fix/falsy-response
Mar 11, 2022
Merged

fix: allow returning, number and boolean as well#65
pi0 merged 6 commits intomainfrom
fix/falsy-response

Conversation

@danielroe
Copy link
Copy Markdown
Member

resolves #48

This enables us to send a response for:

  • boolean
  • number
  • bigint

We may wrongly return a response for functions & symbols. Though likely that's a user error rather than something h3 should check for.

@danielroe danielroe requested a review from pi0 March 10, 2022 23:28
@danielroe danielroe self-assigned this Mar 10, 2022
@pi0
Copy link
Copy Markdown
Member

pi0 commented Mar 11, 2022

It would be better if we check already resolved type against JSON-safe values rather than assuming all values as response

Comment thread src/app.ts Outdated
if (type === 'string') {
return send(res, val, MIMES.html)
} else if (type === 'object' && val !== undefined) {
} else if (['bigint', 'number', 'boolean', 'object'].includes(type)) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For maximum performance, we could avoid iteration and add 4 == checks.

@pi0 pi0 changed the title fix: return non-object values as well fix: allow returning bigint, number and boolean as well Mar 11, 2022
@pi0 pi0 changed the title fix: allow returning bigint, number and boolean as well fix: allow returning, number and boolean as well Mar 11, 2022
@pi0
Copy link
Copy Markdown
Member

pi0 commented Mar 11, 2022

Removed bigint. JSON.parse seems unable to handle bigint by default (https://stackoverflow.com/questions/18755125/node-js-is-there-any-proper-way-to-parse-json-with-large-numbers-long-bigin)

Comment thread src/app.ts Outdated
@pi0 pi0 merged commit 9a01465 into main Mar 11, 2022
@pi0 pi0 deleted the fix/falsy-response branch March 11, 2022 14:17
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.

is it necessary?

2 participants