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
Open

Add partial json type that allows for optional keys #417

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0d52f39
Add partial json type that allows optional values
blink1073 Jul 18, 2017
714c6b1
Update tests
blink1073 Jul 18, 2017
fb2e73a
Fix docstrings + add a further test
vidartf Aug 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 83 additions & 13 deletions packages/coreutils/src/json.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,48 @@ export
type ReadonlyJSONValue = JSONPrimitive | ReadonlyJSONObject | ReadonlyJSONArray;


/**
* A type alias for a partial JSON value.
*/
export
type PartialJSONValue = JSONPrimitive | PartialJSONObject | PartialJSONArray;


/**
* A type definition for a partial JSON object.
*/
export
interface PartialJSONObject { [key: string]: PartialJSONValue | undefined; }


/**
* A type definition for a partial JSON array.
*/
export
interface PartialJSONArray extends Array<PartialJSONValue> { }


/**
* A type definition for a readonly partial JSON object.
*/
export
interface ReadonlyPartialJSONObject { readonly [key: string]: ReadonlyPartialJSONValue | undefined; }


/**
* A type definition for a readonly partial JSON array.
*/
export
interface ReadonlyPartialJSONArray extends ReadonlyArray<ReadonlyPartialJSONValue> { }


/**
* A type alias for a readonly partial JSON value.
*/
export
type ReadonlyPartialJSONValue = JSONPrimitive | ReadonlyPartialJSONObject | ReadonlyPartialJSONArray;


/**
* The namespace for JSON-specific functions.
*/
Expand All @@ -81,7 +123,7 @@ namespace JSONExt {
* @returns `true` if the value is a primitive,`false` otherwise.
*/
export
function isPrimitive(value: ReadonlyJSONValue): value is JSONPrimitive {
function isPrimitive(value: ReadonlyPartialJSONValue): value is JSONPrimitive {
return (
value === null ||
typeof value === 'boolean' ||
Expand All @@ -102,7 +144,11 @@ namespace JSONExt {
export
function isArray(value: ReadonlyJSONValue): value is ReadonlyJSONArray;
export
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
}

export
function isArray(value: ReadonlyPartialJSONValue): boolean {
return Array.isArray(value);
}

Expand All @@ -118,7 +164,11 @@ namespace JSONExt {
export
function isObject(value: ReadonlyJSONValue): value is ReadonlyJSONObject;
export
function isObject(value: ReadonlyJSONValue): boolean {
function isObject(value: PartialJSONValue): value is PartialJSONObject;
export
function isObject(value: ReadonlyPartialJSONValue): value is ReadonlyPartialJSONObject;
export
function isObject(value: ReadonlyPartialJSONValue): boolean {
return !isPrimitive(value) && !isArray(value);
}

Expand All @@ -132,7 +182,7 @@ namespace JSONExt {
* @returns `true` if the values are equivalent, `false` otherwise.
*/
export
function deepEqual(first: ReadonlyJSONValue, second: ReadonlyJSONValue): boolean {
function deepEqual(first: ReadonlyPartialJSONValue, second: ReadonlyPartialJSONValue): boolean {
// Check referential and primitive equality first.
if (first === second) {
return true;
Expand All @@ -154,11 +204,11 @@ namespace JSONExt {

// If they are both arrays, compare them.
if (a1 && a2) {
return deepArrayEqual(first as ReadonlyJSONArray, second as ReadonlyJSONArray);
return deepArrayEqual(first as ReadonlyPartialJSONArray, second as ReadonlyPartialJSONArray);
}

// At this point, they must both be objects.
return deepObjectEqual(first as ReadonlyJSONObject, second as ReadonlyJSONObject);
return deepObjectEqual(first as ReadonlyPartialJSONObject, second as ReadonlyPartialJSONObject);
}

/**
Expand All @@ -169,7 +219,7 @@ namespace JSONExt {
* @returns A deep copy of the given JSON value.
*/
export
function deepCopy<T extends ReadonlyJSONValue>(value: T): T {
function deepCopy<T extends ReadonlyPartialJSONValue>(value: T): T {
// Do nothing for primitive values.
if (isPrimitive(value)) {
return value;
Expand All @@ -187,7 +237,7 @@ namespace JSONExt {
/**
* Compare two JSON arrays for deep equality.
*/
function deepArrayEqual(first: ReadonlyJSONArray, second: ReadonlyJSONArray): boolean {
function deepArrayEqual(first: ReadonlyPartialJSONArray, second: ReadonlyPartialJSONArray): boolean {
// Check referential equality first.
if (first === second) {
return true;
Expand All @@ -212,29 +262,44 @@ namespace JSONExt {
/**
* Compare two JSON objects for deep equality.
*/
function deepObjectEqual(first: ReadonlyJSONObject, second: ReadonlyJSONObject): boolean {
function deepObjectEqual(first: ReadonlyPartialJSONObject, second: ReadonlyPartialJSONObject): boolean {
// Check referential equality first.
if (first === second) {
return true;
}

// Check for the first object's keys in the second object.
for (let key in first) {
if (!(key in second)) {
if (first[key] !== undefined && !(key in second)) {
vidartf marked this conversation as resolved.
Show resolved Hide resolved
return false;
}
}

// Check for the second object's keys in the first object.
for (let key in second) {
if (!(key in first)) {
if (second[key] !== undefined && !(key in first)) {
vidartf marked this conversation as resolved.
Show resolved Hide resolved
return false;
}
}

// Compare the values for equality.
for (let key in first) {
if (!deepEqual(first[key], second[key])) {
// Get the values.
let firstValue = first[key];
let secondValue = second[key];

// If both are undefined, ignore the key.
if (firstValue === undefined && secondValue === undefined) {
continue;
}

// If only one value is undefined, the objects are not equal.
if (firstValue === undefined || secondValue === undefined) {
return false;
}

// Compare the values.
if (!deepEqual(firstValue, secondValue)) {
return false;
}
}
Expand All @@ -260,7 +325,12 @@ namespace JSONExt {
function deepObjectCopy(value: any): any {
let result: any = {};
for (let key in value) {
result[key] = deepCopy(value[key]);
// Ignore undefined values.
let subvalue = value[key];
if (subvalue === undefined) {
continue;
}
result[key] = deepCopy(subvalue);
}
return result;
}
Expand Down
38 changes: 37 additions & 1 deletion packages/coreutils/tests/src/json.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,16 @@ import {
} from 'chai';

import {
JSONArray, JSONExt, JSONObject, JSONPrimitive
JSONArray, JSONExt, JSONObject, JSONPrimitive,
PartialJSONObject
} from '@phosphor/coreutils';


interface IFoo extends PartialJSONObject {
bar?: string;
}


describe('@phosphor/coreutils', () => {

describe('JSONExt', () => {
Expand Down Expand Up @@ -74,6 +80,24 @@ describe('@phosphor/coreutils', () => {
expect(JSONExt.deepEqual({ b: 1 }, { a: 1 })).to.equal(false);
});

it('should handle optional keys on partials', () => {
let a: IFoo = { };
let b: IFoo = { bar: 'a' };
let c: IFoo = { bar: undefined };
expect(JSONExt.deepEqual(a, b)).to.equal(false);
expect(JSONExt.deepEqual(a, c)).to.equal(true);
expect(JSONExt.deepEqual(c, a)).to.equal(true);
});

it('should equate an object to its deepCopy', () => {
let a: IFoo = { };
let b: IFoo = { bar: 'a' };
let c: IFoo = { bar: undefined };
expect(JSONExt.deepEqual(a, JSONExt.deepCopy(a))).to.equal(true);
expect(JSONExt.deepEqual(b, JSONExt.deepCopy(b))).to.equal(true);
expect(JSONExt.deepEqual(c, JSONExt.deepCopy(c))).to.equal(true);
});

});

describe('deepCopy()', () => {
Expand Down Expand Up @@ -110,6 +134,18 @@ describe('@phosphor/coreutils', () => {
expect(v7['c']).to.not.equal(r7['c']);
});

it('should handle optional keys on partials', () => {
let v1: IFoo = { };
let v2: IFoo = { bar: 'a' };
let v3 = { a: false, b: { bar: undefined }};
let r1 = JSONExt.deepCopy(v1);
let r2 = JSONExt.deepCopy(v2);
let r3 = JSONExt.deepCopy(v3);
expect(Object.keys(r1).length).to.equal(0);
expect(v2.bar).to.equal(r2.bar);
expect(Object.keys(r3.b).length).to.equal(0);
});

});

});
Expand Down