-
Notifications
You must be signed in to change notification settings - Fork 168
Add partial json type that allows for optional keys #417
base: master
Are you sure you want to change the base?
Conversation
Note: While the logic in |
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.
LGTM!
function isArray(value: ReadonlyJSONValue): boolean { | ||
function isArray(value: PartialJSONValue): value is PartialJSONArray; | ||
export | ||
function isArray(value: ReadonlyPartialJSONValue): value is ReadonlyPartialJSONArray; |
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.
This can break third party code which is using the functions as type assertion checks, since it's changing the return type. Instead, let's add overloads for each of these functions.
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.
I think the current code only adds overloads, and leaves the return type untouched. It does change the argument type though.
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.
Do you have an example that currently works, but will fail after this change? It would make it a lot easier to get this right.
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.
Note: With the current code, these narrowings are in effect:
// Same as before:
let a = [] as JSONValue;
let b = [] as ReadonlyJSONValue;
if (JSONExt.isArray(a)) {
// a: JSONArray
}
if (JSONExt.isArray(b)) {
// b: ReadonlyJSONArray
}
// New possibilities:
let c = [] as PartialJSONValue;
let d = [] as ReadonlyPartialJSONValue;
if (JSONExt.isArray(c)) {
// c: PartialJSONArray
}
if (JSONExt.isArray(d)) {
// d: ReadonlyPartialJSONArray
}
Refactor of #303 to instead add new partial types that allow undefined value and/or missing keys ("Partial" types in TS jargon).
Some of the type names get a little long, but at least they are explicit.