Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions packages/common/core-interfaces/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,11 @@
"./internal": {
"import": {
"types": "./lib/internal.d.ts",
"default": "./lib/index.js"
"default": "./lib/internal.js"
},
"require": {
"types": "./dist/internal.d.ts",
"default": "./dist/index.js"
"default": "./dist/internal.js"
}
},
"./internal/exposedUtilityTypes": {
Expand Down
2 changes: 1 addition & 1 deletion packages/common/core-interfaces/src/cjs/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
},
"./internal": {
"types": "./internal.d.ts",
"default": "./index.js"
"default": "./internal.js"
},
"./internal/exposedUtilityTypes": "./exposedUtilityTypes.js"
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -871,6 +871,9 @@ export namespace InternalUtilityTypes {
export type JsonSerializableImpl<
T,
Options extends Partial<FilterControls> & {
/**
* See {@link JsonSerializableOptions} for meaning and expected use.
*/
IgnoreInaccessibleMembers?: "ignore-inaccessible-members";
},
TAncestorTypes extends unknown[] = [],
Expand Down Expand Up @@ -903,7 +906,7 @@ export namespace InternalUtilityTypes {
Controls extends FilterControlsWithSubstitution
? /* test for 'any' */ boolean extends (T extends never ? true : false)
? /* 'any' => */ Controls["DegenerateSubstitute"]
: Options["IgnoreInaccessibleMembers"] extends "ignore-inaccessible-members"
: Options extends { IgnoreInaccessibleMembers: "ignore-inaccessible-members" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its unclear how somone is supposed to find the docs on JsonSerializableOptions: IgnoreInaccessibleMembers from here.

Up on line 871 I think you should replace

{
			IgnoreInaccessibleMembers?: "ignore-inaccessible-members";
		}

with Pick<JsonSerializableOptions, "IgnoreInaccessibleMembers">

so its clear those are the same field, and that documentation applies.

THen here you could do:

Suggested change
: Options extends { IgnoreInaccessibleMembers: "ignore-inaccessible-members" }
: Options extends Required<Pick<JsonSerializableOptions, "IgnoreInaccessibleMembers">>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be a cyclic file import unless that type is relocated, which didn't feel the best for readability for the API file. I am trying to keep the exposed options a bit separated from the internals. See also internal FilterControls that is like part of the JsonSerializableOptions -- I don't want to have those interdependent.
But, yes, there are no docs now in this place for that internal option. I will add a comment.

? JsonSerializableFilter<T, Controls, TAncestorTypes, TNextAncestor>
: /* test for non-public properties (class instance type) */
IfNonPublicProperties<
Expand Down
5 changes: 2 additions & 3 deletions packages/common/core-interfaces/src/internal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,8 @@
// eslint-disable-next-line no-restricted-syntax, @typescript-eslint/no-restricted-imports
export * from "./index.js";

// Important: all other exports must be type only exports. In package.json exports,
// index.js is listed as the runtime file. This is done so that all imports are
// using the same outer runtime file. (Could be changed if needed.)
export type { JsonString, JsonStringifyOptions } from "./jsonString.js";
export { JsonStringify, JsonParse } from "./jsonString.js";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see these two are just redefinitions of native functions but feels like they should still go to core-utils to keep this package types-only?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Package cannot be type-only because it has enums.
core-utils does not depend on core-interfaces. If relocated, they could go to client-utils.
Since it can't be type-only and relocation would involve adding test exports to core-interfaces for all of the testing, I am inclined to say this is okay. But I will defer to the prevailing wisdom.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, client-utils is the one I was thinking of.

I can't think of strong reasons to oppose exporting them from here, but don't feel super confident being sole approver. I'd suggest getting more eyes on this. @anthony-murphy comes to mind as someone who might have comments.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really grasping the implications of this change, if there are concerns please bottom-out on those, despite my approval on the PR as a whole

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is mostly a policy / positioning for this package to be type-only and not have any runtime contribution. But that is not the case with these packages anyhow because enums or enum-like types. I do not want us to start down a slippery slope. I am convinced the Json* type filters and JsonString belong here. I think it is okay to has JsonStringify and JsonParse colocated when there are just retypings of existing functions.


export type { JsonTypeToOpaqueJson, OpaqueJsonToJsonType } from "./jsonUtils.js";

Expand Down
12 changes: 10 additions & 2 deletions packages/common/core-interfaces/src/jsonSerializable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,17 @@ export interface JsonSerializableOptions {

/**
* When set, inaccessible (protected and private) members throughout type T are
* ignored as if not present.
* ignored as if not present. Otherwise, inaccessible members are considered
* an error (type checking will mention `SerializationErrorPerNonPublicProperties`).
*
* The default value is not present.
* @remarks
* For this option to be set and accurately filter inaccessible members, all
* inaccessible members (if any) must be either inherited, symbol keyed, or
* non-enumerable.
*
* The default is that `IgnoreInaccessibleMembers` property is not specified,
* which means that inaccessible members are considered an error, even if
* they would not be serialized at runtime.
*/
IgnoreInaccessibleMembers?: "ignore-inaccessible-members";
}
Expand Down
126 changes: 126 additions & 0 deletions packages/common/core-interfaces/src/jsonString.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
/*!
* Copyright (c) Microsoft Corporation and contributors. All rights reserved.
* Licensed under the MIT License.
*/

// eslint-disable-next-line @typescript-eslint/consistent-type-imports -- incorrect rule: misunderstands `declare`d types.
import type { BrandedType } from "./brandedType.js";
import type { InternalUtilityTypes } from "./exposedInternalUtilityTypes.js";
import type { JsonDeserialized } from "./jsonDeserialized.js";
import type { JsonSerializable, JsonSerializableOptions } from "./jsonSerializable.js";

/**
* Brand for JSON that has been stringified.
*
* Usage: Intersect with another type to apply branding.
*
* @sealed
*/
declare class JsonStringBrand<T> extends BrandedType<JsonString<unknown>> {
public toString(): string;
protected readonly EncodedValue: T;
private constructor();
}

/**
* Distributes `JsonStringBrand` over union elements of T.
*
* @remarks
* This is useful to allow `JsonString<A | B>` to be assigned to `JsonString<A> | JsonString<B>`.
*
* The downside is that enums are expanded to union of members and thus cannot be
* reconstituted exactly (even if IntelliSense shows the original enum type). This
* can be removed if exact enum preservation is found to be more important.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we like avoiding enums anyway so seems like the right priority

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that helper has that problem. Then we use the other helper - lines below to manage that.

*/
type DistributeJsonStringBrand<T> = T extends unknown ? JsonStringBrand<T> : never;

/**
* Distributes branding over union elements of T unless result could prevent T
* from being reconstituted (as in the case of an enum type), in which case it
* falls back to a single JsonStringBrand for the entire T.
*
* @remarks
* Note that an enum unioned with anything else will be distributed. It seems
* however that TypeScript can/will reconstitute the enum type in that case.
*/
type BrandForJsonString<T> = InternalUtilityTypes.IfSameType<
DistributeJsonStringBrand<T> extends JsonStringBrand<infer U> ? U : never,
T,
DistributeJsonStringBrand<T>,
JsonStringBrand<T>
>;

/**
* Branded `string` for JSON that has been stringified.
*
* @remarks
*
* Use {@link JsonStringify} to encode JSON producing values of this type and
* {@link JsonParse} to decode them.
*
* For custom encoding/decoding:
*
* - cast to with `as unknown as JsonString<T>` when value of type `T` has been stringified.
*
* - use a form of {@link JsonDeserialized} for safety when parsing.
*
* @sealed
* @internal
*/
export type JsonString<T> = string & BrandForJsonString<T>;

/**
* Compile options for {@link JsonStringify}.
*
* @remarks
* This only impacts type checking -- it has no impact on runtime.
*
* The options are currently a subset of {@link JsonSerializableOptions}, specifically
* only `IgnoreInaccessibleMembers` is supported.
*
* No instance of this should ever exist at runtime.
*
* @privateRemarks
* Consider adding `AllowUnknown` option to allow precisely `unknown` types to
* be passed through. With `unknown` expected successful serialization could not
* be checked at compile time. At deserialization time, `unknown` does not
* guarantee any type and thus allowing does not erode type safety.
*
* @internal
*/
export type JsonStringifyOptions = Pick<JsonSerializableOptions, "IgnoreInaccessibleMembers">;

/**
* Performs basic JSON serialization using `JSON.stringify` and brands the result as {@link JsonString}`<T>`.
*
* @remarks
* Parameter `value` must be JSON-serializable and thus type T is put through filter {@link JsonSerializable}.
*
* @internal
*/
export const JsonStringify = JSON.stringify as <
T,
Options extends JsonStringifyOptions = Record<never, never>,
>(
value: JsonSerializable<
T,
// Make sure only options that are known are passed through.
Pick<Options, Extract<keyof JsonStringifyOptions, keyof Options>>
>,
) => JsonString<T>;

/**
* Performs basic JSON parsing using `JSON.parse` given a {@link JsonString}`<T>` (`string`).
*
* @remarks
* Return type is filtered through {@link JsonDeserialized}`<T>` for best accuracy.
*
* Note that `JsonParse` cannot verify at runtime that the input is valid JSON
* or that it matches type T. It is the caller's responsibility to ensure that
* the input is valid JSON and the output conforms to the expected type.
*
* @internal
*/
export const JsonParse = JSON.parse as <T extends JsonString<unknown>>(
text: T,
) => T extends JsonString<infer U> ? JsonDeserialized<U> : unknown;
69 changes: 44 additions & 25 deletions packages/common/core-interfaces/src/test/jsonDeserialized.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import type {
BrandedKey,
BrandedString,
DecodedValueDirectoryOrRequiredState,
DeserializedOpaqueSerializableInRecursiveStructure,
DeserializedOpaqueSerializableAndDeserializedInRecursiveStructure,
DirectoryOfValues,
ObjectWithOptionalRecursion,
} from "./testValues.js";
Expand Down Expand Up @@ -218,6 +220,12 @@ import {
opaqueSerializableObjectExpectingBigintSupport,
opaqueDeserializedObjectExpectingBigintSupport,
opaqueSerializableAndDeserializedObjectExpectingBigintSupport,
jsonStringOfString,
jsonStringOfObjectWithArrayOfNumbers,
jsonStringOfStringRecordOfNumbers,
jsonStringOfStringRecordOfNumberOrUndefined,
jsonStringOfBigInt,
jsonStringOfUnknown,
} from "./testValues.js";

import type { IFluidHandle } from "@fluidframework/core-interfaces";
Expand All @@ -227,7 +235,6 @@ import type {
JsonTypeWith,
NonNullJsonObjectWith,
OpaqueJsonDeserialized,
OpaqueJsonSerializable,
} from "@fluidframework/core-interfaces/internal/exposedUtilityTypes";

/**
Expand Down Expand Up @@ -375,6 +382,36 @@ describe("JsonDeserialized", () => {
assertIdenticalTypes(resultRead, brandedString);
assertNever<AnyLocations<typeof resultRead>>();
});
it("`JsonString<string>`", () => {
const resultRead = passThru(jsonStringOfString);
assertIdenticalTypes(resultRead, jsonStringOfString);
assertNever<AnyLocations<typeof resultRead>>();
});
it("`JsonString<{ arrayOfNumbers: number[] }>`", () => {
const resultRead = passThru(jsonStringOfObjectWithArrayOfNumbers);
assertIdenticalTypes(resultRead, jsonStringOfObjectWithArrayOfNumbers);
assertNever<AnyLocations<typeof resultRead>>();
});
it("`JsonString<Record<string, number>>`", () => {
const resultRead = passThru(jsonStringOfStringRecordOfNumbers);
assertIdenticalTypes(resultRead, jsonStringOfStringRecordOfNumbers);
assertNever<AnyLocations<typeof resultRead>>();
});
it("`JsonString<Record<string, number | undefined>>`", () => {
const resultRead = passThru(jsonStringOfStringRecordOfNumberOrUndefined);
assertIdenticalTypes(resultRead, jsonStringOfStringRecordOfNumberOrUndefined);
assertNever<AnyLocations<typeof resultRead>>();
});
it("JsonString<bigint>", () => {
const resultRead = passThru(jsonStringOfBigInt);
assertIdenticalTypes(resultRead, jsonStringOfBigInt);
assertNever<AnyLocations<typeof resultRead>>();
});
it("JsonString<unknown>", () => {
const resultRead = passThru(jsonStringOfUnknown);
assertIdenticalTypes(resultRead, jsonStringOfUnknown);
assertNever<AnyLocations<typeof resultRead>>();
});
});

describe("unions with unsupported primitive types preserve supported types", () => {
Expand Down Expand Up @@ -523,11 +560,15 @@ describe("JsonDeserialized", () => {
});
it("array of functions with properties becomes ({...}|null)[]", () => {
const resultRead = passThru(arrayOfFunctionsWithProperties, [null]);
// Note: a function with properties always results in `null`, but a property bag
// with a function is the property bag. Allow either to avoid order-based logic.
assertIdenticalTypes(resultRead, createInstanceOf<({ property: number } | null)[]>());
assertNever<AnyLocations<typeof resultRead>>();
});
it("array of objects and functions becomes ({...}|null)[]", () => {
const resultRead = passThru(arrayOfObjectAndFunctions, [{ property: 6 }]);
// Note: a function with properties always results in `null`, but a property bag
// with a function is the property bag. Allow either to avoid order-based logic.
assertIdenticalTypes(resultRead, createInstanceOf<({ property: number } | null)[]>());
assertNever<AnyLocations<typeof resultRead>>();
});
Expand Down Expand Up @@ -1726,15 +1767,6 @@ describe("JsonDeserialized", () => {
});
it("object with OpaqueJsonSerializable<unknown> in recursion is unrolled one time with OpaqueJsonDeserialized", () => {
const resultRead = passThru(opaqueSerializableInRecursiveStructure);
interface DeserializedOpaqueSerializableInRecursiveStructure {
items: {
[x: string | number]:
| OpaqueJsonDeserialized<DirectoryOfValues<OpaqueJsonSerializable<unknown>>>
| {
value?: OpaqueJsonDeserialized<unknown>;
};
};
}
assertIdenticalTypes(
resultRead,
createInstanceOf<DeserializedOpaqueSerializableInRecursiveStructure>(),
Expand All @@ -1757,22 +1789,9 @@ describe("JsonDeserialized", () => {
// It might be better to preserve the intersection and return original type.
it("object with OpaqueJsonSerializable<unknown> & OpaqueJsonDeserialized<unknown> in recursion is unrolled one time with OpaqueJsonDeserialized", () => {
const resultRead = passThru(opaqueSerializableAndDeserializedInRecursiveStructure);
interface DeserializedOpaqueSerializableInRecursiveStructure {
items: {
[x: string | number]:
| OpaqueJsonDeserialized<
DirectoryOfValues<
OpaqueJsonSerializable<unknown> & OpaqueJsonDeserialized<unknown>
>
>
| {
value?: OpaqueJsonDeserialized<unknown>;
};
};
}
assertIdenticalTypes(
resultRead,
createInstanceOf<DeserializedOpaqueSerializableInRecursiveStructure>(),
createInstanceOf<DeserializedOpaqueSerializableAndDeserializedInRecursiveStructure>(),
);
assertNever<AnyLocations<typeof resultRead>>();
const transparentResult = revealOpaqueJson(resultRead);
Expand All @@ -1781,7 +1800,7 @@ describe("JsonDeserialized", () => {
createInstanceOf<{
items: {
[x: string | number]:
| DeserializedOpaqueSerializableInRecursiveStructure
| DeserializedOpaqueSerializableAndDeserializedInRecursiveStructure
| {
value?: JsonTypeWith<never>;
};
Expand Down
Loading
Loading