-
Notifications
You must be signed in to change notification settings - Fork 24
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 78: Serializer.encode throws for null and undefined #79
Conversation
I made the decision to encode null based on my own usage: Retrieving data using Prisma ORM in which nullable database columns are returned as null. I see it as too much maintenance effort to have to convert each nullable column into an Option, especially in an existing codebase where most usages expect This approach also brings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for your PR! made a small comment
src/Serializer.ts
Outdated
@@ -6,7 +6,7 @@ export const encode = (value: any, indent?: number | undefined) => { | |||
return JSON.stringify( | |||
value, | |||
(key, value) => { | |||
if (typeof value[BOXED_TYPE] === "string") { | |||
if (typeof value?.[BOXED_TYPE] === "string") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's maybe rather use the following to maximize browser compatibility
if (value == null) {
return value
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been a while since I had to think about syntax support of older browsers! 😅
I had a niggling that the Typescript compiler actually compiled out optional chaining - depending on the build target - and ran a test build. It did indeed turn the optional chaining into the following:
null==n?n:"string"==typeof n[o]
Then to really satisfy my curiosity, I tried your suggestion and it actually compiled to the exact same output!
thanks for your PR! available in v2.3.1 |
This resolves #78.
This does diverge from previous behavior by encoding null.