Skip to content
This repository has been archived by the owner on Nov 6, 2019. It is now read-only.

Add partial json type that allows for optional keys #417

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vidartf
Copy link
Contributor

@vidartf vidartf commented Aug 15, 2019

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.

@vidartf vidartf changed the title Partial json Add partial json type that allows for optional keys Aug 15, 2019
@vidartf
Copy link
Contributor Author

vidartf commented Aug 15, 2019

Note: While the logic in JSONExt changed to support the new type, I do not think the behavior for the old type should differ (potentially a minor performance hit). If this is not the case, or the performance hit is not acceptable, we could duplicate that into PartialJSONExt as well.

Copy link
Contributor

@blink1073 blink1073 left a 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;
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants