-
Notifications
You must be signed in to change notification settings - Fork 125
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
Add strong types for JSON.stringify #21
Open
sstur
wants to merge
5
commits into
mattpocock:main
Choose a base branch
from
sstur:addJsonStringify
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
f710a3a
Add strong types for JSON.stringify
sstur 2205b69
Add clarifying comment
sstur c54739a
Add another overload to better handle undefined
sstur 73df554
Use "as" to prevent TS from auto-narrowing
sstur e25f6e7
Update to account for function being treated as undefined
sstur File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
interface JSON { | ||
/** | ||
* Converts a JavaScript value to a JavaScript Object Notation (JSON) string. | ||
* @param value A JavaScript value, usually an object or array, to be converted. | ||
* @param replacer (Not Used) | ||
* @param space (Not Used) | ||
*/ | ||
stringify( | ||
value: undefined | ((...args: any[]) => any), | ||
replacer?: any, | ||
space?: any, | ||
): undefined; | ||
/** | ||
* Converts a JavaScript value to a JavaScript Object Notation (JSON) string. | ||
* @param value A JavaScript value, usually an object or array, to be converted. | ||
* @param replacer A function that transforms the results. | ||
* @param space Adds indentation, white space, and line break characters to the return-value JSON text to make it easier to read. | ||
*/ | ||
stringify<T = any>( | ||
value: T, | ||
replacer?: (this: any, key: string, value: any) => any, | ||
space?: string | number, | ||
): undefined extends T | ||
? string | undefined | ||
: ((...args: any[]) => any) extends T | ||
? string | undefined | ||
: string; | ||
/** | ||
* Converts a JavaScript value to a JavaScript Object Notation (JSON) string. | ||
* @param value A JavaScript value, usually an object or array, to be converted. | ||
* @param replacer An array of strings and numbers that acts as an approved list for selecting the object properties that will be stringified. | ||
* @param space Adds indentation, white space, and line break characters to the return-value JSON text to make it easier to read. | ||
*/ | ||
stringify<T = any>( | ||
value: T, | ||
replacer?: (number | string)[] | null, | ||
space?: string | number, | ||
): undefined extends T | ||
? string | undefined | ||
: ((...args: any[]) => any) extends T | ||
? string | undefined | ||
: string; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
import { doNotExecute, Equal, Expect } from "./utils"; | ||
|
||
doNotExecute(() => { | ||
const result = JSON.stringify(undefined); | ||
type tests = [Expect<Equal<typeof result, undefined>>]; | ||
}); | ||
|
||
doNotExecute(() => { | ||
// A function is treated as undefined by the JSON serializer | ||
const fn = () => {}; | ||
const result = JSON.stringify(fn); | ||
type tests = [Expect<Equal<typeof result, undefined>>]; | ||
}); | ||
|
||
doNotExecute(() => { | ||
// If the incoming value can possibly be undefined then the return type can | ||
// possibly be undefined | ||
const result = JSON.stringify(5 as undefined | number); | ||
type tests = [Expect<Equal<typeof result, string | undefined>>]; | ||
}); | ||
|
||
doNotExecute(() => { | ||
// If the incoming value can possibly be a function then the return type can | ||
// possibly be undefined | ||
const result = JSON.stringify(5 as number | (() => number)); | ||
type tests = [Expect<Equal<typeof result, string | undefined>>]; | ||
}); | ||
|
||
doNotExecute(() => { | ||
// If the type of the incoming value is `any` we can't assume anything about | ||
// it so return the broadest possible type | ||
const result = JSON.stringify(undefined as any); | ||
sstur marked this conversation as resolved.
Show resolved
Hide resolved
|
||
type tests = [Expect<Equal<typeof result, string | undefined>>]; | ||
}); | ||
|
||
doNotExecute(() => { | ||
// If the type of the incoming value is `unknown` we can't assume anything | ||
// about it so return the broadest possible type | ||
const result = JSON.stringify(undefined as unknown); | ||
type tests = [Expect<Equal<typeof result, string | undefined>>]; | ||
}); | ||
|
||
doNotExecute(() => { | ||
// If the type of the incoming value is "non-nullish" we can be sure it's not | ||
// undefined, but we can't be sure it isn't a function (treated as undefined) | ||
// so return the broadest possible type. | ||
const fn = () => {}; | ||
const result = JSON.stringify(fn as Object); | ||
type tests = [Expect<Equal<typeof result, string | undefined>>]; | ||
}); | ||
|
||
doNotExecute(() => { | ||
// If the type of the incoming value contains "object" (meaning non-primitive) | ||
// we can't be sure it isn't a function (treated as undefined) so return the | ||
// broadest possible type. | ||
const value = null as null | string | number | boolean | object; | ||
const result = JSON.stringify(value); | ||
type tests = [Expect<Equal<typeof result, string | undefined>>]; | ||
}); | ||
|
||
doNotExecute(() => { | ||
// If the type of the incoming value contains "Record<K, any>" (in which K is | ||
// any valid index type, e.g. string or symbol) we can't be sure it isn't a | ||
// function so return the broadest possible type. | ||
const value = null as null | string | Record<string, any>; | ||
const result = JSON.stringify(value); | ||
type tests = [Expect<Equal<typeof result, string | undefined>>]; | ||
}); | ||
|
||
doNotExecute(() => { | ||
const value = null as null | string | Record<string, unknown> | any[]; | ||
const result = JSON.stringify(value); | ||
type tests = [Expect<Equal<typeof result, string>>]; | ||
}); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
functions are also resolved as undefined
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.
Oh, this is a good catch! I didn't even realize that. Checking into it...
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.
Hmm, so it turns out this strange behavior of JSON.stringify() treating a function as undefined sorta throws a wrench in the works, because a function can be assigned to
Record<string, any>
, meaning it's hard to be certain that we have an object that is not a function.I've update this PR to account for that possibility but be aware this makes it so that many more situations will fall back to
string | undefined
vs juststring
.I think we should consider ignoring the "function treated as undefined" edge case and give up some soundness for developer ergonomics. It would still be far more correct than the in-built types, but would not be perfectly sound (choosing pragmatism over soundness is the long standing posture of TS anyway).
Thoughts @mattpocock?
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.
To put this another way:
Many legit values (that are never intended to be a function) in the real world will likely have the type
Record<string, any>
and if we go for soundest possible types forJSON.stringify()
then passing in those real-world values will result instring | undefined
being returned, likely confusing the average TS dev who doesn't realize why. Is this a tradeoff we want to make?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.
Could you add an overload to catch it being a function? And then only return undefined.
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 is right to assume that a value typed
Record<string, any>
could be a function at runtime:playground
This would actually be useful, it will alert the developer to a case when they have accidentally used
stringify
in an invalid case.That argument could be applied to literally any value:
I don't think it's a far-fetched idea to specialize this function, even the
toJSON
case could be accounted for in the conditional return type.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.
There are two problems with this argument.
The type
Record<string, any>
usesany
, which disables type checking. Change the type toRecord<string, unknown>
instead and you'll see the type error.For any type
A
, the typeRecord<string, A>
should be assignable toRecord<string, unknown>
because the typeA
is in a covariant position. The type() => void
is not assignable toRecord<string, unknown>
. This means that() => void
is not assignable toRecord<string, A>
for some typeA
, i.e. functions are not records.() => void
being assignable toRecord<string, any>
should be considered a bug. In fact, I went ahead and raised a bug issue for it.() => void
is assignable toRecord<string, any>
microsoft/TypeScript#53484Hopefully, we'll get some clarity on why this issue is occurring. If we get an official answer that this is indeed the expected behavior then I'll be happy to concede this point to you.
That being said, I can see how
Object.assign(() => {}, someRecord)
would have the type(() => void) & Record<string, unknown>
which is indeed assignable toRecord<string, unknown>
. Thus,Record<string, unknown>
could indeed be a function at runtime. All the more reason to not specialize the return type ofJSON.stringify
.You can use a linter to catch that error.
typescript-eslint
supports type-aware linting rules. Even better, a linter will inform you that you've made a mistake. In an IDE, it will show you squiggly lines indicating a warning or an error.Anyway, this is not a strong argument for having specialized return types. It's not worth the added complexity, especially when you have another tool that can solve this problem for you better.
Except, in the case of
JSON.stringify
we're only writing the type definition and not the implementation of the function. So, we have to be extra careful when writing the type definition because it won't be verified by the compiler. Thus, it's easy to have edge cases which are incorrect. Going back to my earlier example, stringifying any object withtoJSON
could potentially returnundefined
.Writing the type definitions for handling all the edge cases will be very complex. So, even if you do get it correct, which is a very big if, you still need to think whether it's worth all the effort. IMHO, it's not worth the effort.
Just keep it simple and return
string | undefined
. You get added safety, and it's not much of an inconvenience for developers who are confident that the return type is onlystring
. If you don't want to use the non-null assertion operator because it's unsafe, you could always just throw an error when you getundefined
.Please, be my guest. I would love to see the type definitions required for handling all the edge cases.
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.
@sstur @KotlinIsland Created my own PR for adding type definitions for
JSON.stringify
, #124.Would greatly appreciate any and all feedback.
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.
the example he was responding to used
Record<string, any>
just because it shouldn't be specialized in that case doesn't mean it shouldn't be specialized in other cases. for example primitives such as
string
andnumber
can be specialized to always returnstring
because we know there's no way they could be functionsi don't get why you'd use a linter for an error that the compiler can detect. imo we should aim to catch as many errors at compiletime as possible, to reduce the need for additional tools like eslint
i agree, but that doesn't mean we shouldn't do it at all. as long as we have tests and verify that each edge case is covered, i see no reason not to do it
i don't think it's that complicated at all. here's my attempt:
sure it looks gross, but if you're used to conditional types it's just four if statements and a union (and i'm sure it could be rewritten in a way that looks nicer. in fact it looks like you already did in #124). perhaps i missed something but i can't imagine it getting much more complicated than this.
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.
@DetachHead I agree. I changed my mind and solved it too, 2 days ago. Created a separate PR for it. See, #124. My PR improves both
JSON.parse
andJSON.stringify
. It even provides strong types for thereviver
inJSON.parse
and thereplacer
function inJSON.stringify
. Furthermore, it correctly handles values with thetoJSON
method. Finally, it passes all the tests in this PR. I would highly recommend that you check it out. Would love any and all feedback.