feat(client-core-interfaces): JsonString
#25501
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new JsonString<T> type that represents a branded string containing JSON-serialized data of type T. The feature provides type-safe JSON serialization and deserialization utilities with comprehensive type validation at compile time.
Key changes:
- Introduces
JsonString<T>branded type withJsonStringifyandJsonParseutilities - Adds extensive test coverage for type behavior and edge cases
- Updates package.json exports to properly expose the new internal module
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/common/core-interfaces/src/jsonString.ts | Core implementation of JsonString type, JsonStringify and JsonParse utilities |
| packages/common/core-interfaces/src/test/jsonString.spec.ts | Type compatibility tests for JsonString variance behavior |
| packages/common/core-interfaces/src/test/jsonStringifyAndParse.spec.ts | Comprehensive functional tests for JsonStringify/JsonParse with edge cases |
| packages/common/core-interfaces/src/test/testValues.ts | Test data additions for JsonString examples |
| packages/common/core-interfaces/src/test/jsonSerializable.spec.ts | Added JsonString type compatibility tests |
| packages/common/core-interfaces/src/test/jsonDeserialized.spec.ts | Added JsonString deserialization tests and type cleanup |
| packages/common/core-interfaces/src/internal.ts | Export declarations for new JsonString functionality |
| packages/common/core-interfaces/package.json | Updated export paths for internal module |
| packages/common/core-interfaces/src/cjs/package.json | Updated CommonJS export paths for internal module |
Comments suppressed due to low confidence (3)
packages/common/core-interfaces/src/test/jsonStringifyAndParse.spec.ts:1
- [nitpick] The
replaceBigIntfunction is used here but its definition and behavior are not clear from the context. Consider adding a comment explaining what this replacer function does for BigInt serialization.
/*!
packages/common/core-interfaces/src/test/jsonStringifyAndParse.spec.ts:1
- [nitpick] This comment and the similar one on lines 562-564 explain complex type behavior but could be more precise. Consider clarifying the specific conditions under which functions with properties become
nullvs when they preserve properties.
/*!
packages/common/core-interfaces/src/test/jsonStringifyAndParse.spec.ts:1
- [nitpick] This comment and the similar one on lines 562-564 explain complex type behavior but could be more precise. Consider clarifying the specific conditions under which functions with properties become
nullvs when they preserve properties.
/*!
| export const JsonStringify = JSON.stringify as < | ||
| T, | ||
| // eslint-disable-next-line @typescript-eslint/ban-types -- `Record<string, never>` is not sufficient replacement for empty object. | ||
| Options extends JsonStringifyOptions = {}, | ||
| >( | ||
| value: JsonSerializable<T, Options>, | ||
| ) => JsonString<T>; |
There was a problem hiding this comment.
[nitpick] The type assertion as is used to cast JSON.stringify to the branded return type. While this works, consider adding a comment explaining why this casting is safe and necessary for the branded type system.
|
See #25522 for example simple planned use. |
| // 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"; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
jason-ha
force-pushed
the
core/JsonString
branch
from
53f528a to
28ca32f
Compare
alexvy86
left a comment
alexvy86
left a comment
There was a problem hiding this comment.
Haven't looked at jsonStringifyAndParse.spec.ts. Just minor comments on the rest, and disclaimer that I feel like an ok reviewer but not an expert in this typing black-magic space :).
with `JsonStringify` and `JsonParse` utilities
Test cases added:
```text
JsonString
✔ `JsonString<string>` is assignable to `string`
✔ `string` is not assignable to `JsonString<unknown>`
✔ object is not assignable to `JsonString<unknown>`
is covariant over T
✔ `JsonString<"literal">` is assignable to `JsonString<string>`
✔ `JsonString<Record<string, number>>` is assignable to `JsonString<Record<string, number | undefined>>`
✔ `JsonString<Record<string, number | undefined>>` is assignable to `JsonString<Record<string, unknown>>`
✔ `JsonString<bigint>` is assignable to `JsonString<unknown>`
is not contravariant over T
✔ `JsonString<string>` is not assignable to `JsonString<"literal">`
✔ `JsonString<Record<string, number | undefined>>` is not assignable to `JsonString<Record<string, number>>`
✔ `JsonString<unknown>` is not assignable to `JsonString<bigint>`
JsonStringify and JsonParse
expected usage
supports primitive types
✔ `boolean`
...
✔ branded `string`
✔ `JsonString<string>`
✔ `JsonString<{ arrayOfNumbers: number[] }>`
✔ `JsonString<Record<string, number>>`
✔ `JsonString<Record<string, number | undefined>>`
✔ `JsonString<bigint>`
✔ `JsonString<unknown>`
supports literal types
✔ `true`
✔ `false`
✔ `0`
✔ "string"
✔ `null`
✔ object with literals
✔ array of literals
✔ tuple of literals
✔ specific numeric enum value
✔ specific string enum value
✔ specific const heterogenous enum value
✔ specific computed enum value
supports array types
✔ array of `number`s
✔ readonly array of `number`s
✔ readonly array of simple objects
supports object types
✔ empty object
✔ object with `never`
✔ object with `boolean`
...
✔ `string`|`number` indexed record of `string`s
✔ `string`|`number` indexed record of objects
✔ templated record of `numbers`
✔ `string` indexed record of `number`|`string`s with known properties
✔ `string`|`number` indexed record of `strings` with known `number` property (unassignable)
✔ branded-`string` indexed of `boolean`s
...
✔ object with possible type recursion through union
✔ object with optional type recursion
✔ object with deep type recursion
✔ object with alternating type recursion
✔ simple non-null object json (NonNullJsonObjectWith<never>)
✔ simple read-only non-null object json (ReadonlyNonNullJsonObjectWith<never>)
✔ non-const enums
✔ object with `readonly`
✔ object with getter implemented via value
✔ object with setter implemented via value
✔ object with matched getter and setter implemented via value
✔ object with mismatched getter and setter implemented via value
class instance
✔ with public data (just cares about data)
object with optional property
✔ without property
✔ with undefined value
✔ with defined value
opaque Json types
✔ opaque serializable object
✔ opaque deserialized object
✔ opaque serializable and deserialized object
✔ opaque serializable unknown
✔ opaque deserialized unknown
✔ opaque serializable and deserialized unknown
✔ object with opaque serializable unknown
✔ object with opaque deserialized unknown
✔ recursive type with opaque serializable unknown
✔ recursive type with opaque deserialized unknown
✔ recursive type with opaque serializable and deserialized unknown
✔ recursive branded indexed object with OpaqueJsonDeserialized<unknown>
supports union types
✔ simple json (JsonTypeWith<never>)
✔ simple read-only json (ReadonlyJsonTypeWith<never>)
with NOT fully supported object types
✔ object with self reference throws on serialization
known defect expectations
✔ sparse array of supported types
✔ object with sparse array of supported types
getters and setters allowed but do not propagate
✔ object with `readonly` implemented via getter
✔ object with getter
✔ object with setter
✔ object with matched getter and setter
✔ object with mismatched getter and setter
class instance
with `ignore-inaccessible-members`
✔ with private data ignores private data (that propagates)
invalid input usage
assumptions
✔ const enums are never readable
unsupported types cause compiler error
✔ `undefined`
✔ `unknown`
✔ `symbol`
✔ `unique symbol`
✔ `bigint`
✔ function
✔ function with supported properties
✔ object and function
✔ object with function with supported properties
✔ object with object and function
✔ function with class instance with private data
✔ function with class instance with public data
✔ class instance with private data and is function
✔ class instance with public data and is function
✔ `object` (plain object)
✔ `void`
✔ branded `object`
✔ branded object with `string`
unions with unsupported primitive types
✔ `string | symbol`
✔ `bigint | string`
✔ `bigint | symbol`
✔ `number | bigint | symbol`
array
✔ array of `bigint`s
✔ array of `symbol`s
...
object
✔ object with exactly `bigint`
✔ object with optional `bigint`
✔ object with exactly `symbol`
...
✔ `string` indexed record of `unknown`
✔ `Partial<>` `string` indexed record of `unknown`
✔ `Partial<>` `string` indexed record of `numbers`
✔ `Partial<>` templated record of `numbers`
✔ object with recursion and `symbol`
✔ function object with recursion
✔ object and function with recursion
✔ nested function object with recursion
✔ nested object and function with recursion
✔ object with inherited recursion extended with unsupported properties
object with `undefined`
✔ as exact property type
✔ in union property
...
object with required `unknown`
✔ as exact property type
✔ as exact property type adjacent to recursion
✔ as exact property type in recursion
of class instance
✔ with private data
✔ with private method
✔ with private getter
✔ with private setter
✔ with public method
✔ with private data in optional recursion
opaque Json types requiring extra allowed types
✔ opaque serializable object with `bigint`
✔ opaque deserialized object with `bigint`
✔ opaque serializable and deserialized object with `bigint`
✔ opaque serializable object with number array expecting `bigint` support
✔ opaque deserialized object with number array expecting `bigint` support
✔ opaque serializable and deserialized object with number array expecting `bigint` support
common class instances
✔ Map
✔ ReadonlyMap
✔ Set
✔ ReadonlySet
Fluid types
✔ `IFluidHandle`
✔ object with `IFluidHandle`
special cases
✔ explicit `any` generic still limits allowed types
`number` edge cases
supported
✔ MIN_SAFE_INTEGER
✔ MAX_SAFE_INTEGER
✔ MIN_VALUE
✔ MAX_VALUE
resulting in `null`
✔ NaN
✔ +Infinity
✔ -Infinity
JsonDeserialized
positive compilation tests
supported primitive types are preserved
✔ `JsonString<string>`
✔ `JsonString<{ arrayOfNumbers: number[] }>`
✔ `JsonString<Record<string, number>>`
✔ `JsonString<Record<string, number | undefined>>`
✔ JsonString<bigint>
✔ JsonString<unknown>
JsonSerializable
positive compilation tests
supported primitive types
✔ `JsonString<string>`
✔ `JsonString<{ arrayOfNumbers: number[] }>`
✔ `JsonString<Record<string, number>>`
✔ `JsonString<Record<string, number | undefined>>`
✔ JsonString<bigint>
✔ JsonString<unknown>
```
jason-ha
force-pushed
the
core/JsonString
branch
from
532fec0 to
4da8ea2
Compare
| * When set, inaccessible (protected and private) members throughout type T are | ||
| * ignored as if not present. Otherwise, inaccessible members are considered | ||
| * an error (type checking will mention `SerializationErrorPerNonPublicProperties`). |
There was a problem hiding this comment.
| * When set, inaccessible (protected and private) members throughout type T are | |
| * ignored as if not present. Otherwise, inaccessible members are considered | |
| * an error (type checking will mention `SerializationErrorPerNonPublicProperties`). | |
| * When set, inaccessible (protected and private) members throughout type T are | |
| * ignored when computing derived types. | |
| * This has no impact on how the such fields are handled at runtime: | |
| * For this setting to match runtime behavior all public fields must be enumerable own properties, | |
| * and all inaccessible ones (if any) must be either inherited, symbol keyed or non-enumerable | |
| * When this option is not set, inaccessible members are considered | |
| * an type error (type checking will mention `SerializationErrorPerNonPublicProperties`). |
There was a problem hiding this comment.
This setting shouldn't have anything to do with public properties enumerable or not. But that comment is appropriate for JsonStringify assuming that is requirement of JSON.stringify.
packages/common/core-interfaces/src/test/jsonStringifyAndParse.spec.ts
Outdated
Show resolved
Hide resolved
from JsonStringify to JsonSerializable
and leverage for JsonStringifyOptions while there are no others.
| ObjectWithOptionalRecursion, | ||
| ObjectWithSymbolOrRecursion, | ||
| } from "./testValues.js"; | ||
| // Note: some values are commented out as not interesting to add coverage for (but acknowledge they exist to test). |
There was a problem hiding this comment.
These commented out ones may grow stale over time. How about including them in the imports, and then actually referencing them in a test block that does nothing, or even is skipped.
There was a problem hiding this comment.
That would break my use pattern where I diff the list to see what is used or not across some of the tests.
I admit that it feels like there could be something better. It was developed organically with just two files to start and has grown to three files. I don't know what is better but also haven't invested so much time in it.
Per the use pattern it is okay if the list is stale b/c there is a mechanism to check.
See #25596 where I just bring test case sets more in-line.
There was a problem hiding this comment.
how about you import * as testValues then do something like Object.entries(testValues).filter((t) =>! skipped.has(t[0]))
There was a problem hiding this comment.
Worth trying IMO, can be in a subsequent PR
There was a problem hiding this comment.
I don't quite follow yet but would definitely prefer a follow-up to now.
markfields
left a comment
markfields
left a comment
There was a problem hiding this comment.
Thanks for doing such a mind-bogglingly thorough job here! Looking forward to using this for clarifying op handling code in ContainerRuntime. Would have helped build confidence in some handle serialization changes we made over a year ago too, if we'd had it.
- Use `as` for `JsonParse` to support `JSON.parse` changing to return `unknown` in future and to be consistent with `JsonStringify` declaration. - Add test case for "object with optional undefined" - Cleanup class instance test cases (including those commented out)
…| JsonString<B>` to `JsonDeserialized<A | B>`
…assigned to `JsonString<A> | JsonString<B>` at the cost of not exactly preserving enum type identities.
| ? /* 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" } |
There was a problem hiding this comment.
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:
| : Options extends { IgnoreInaccessibleMembers: "ignore-inaccessible-members" } | |
| : Options extends Required<Pick<JsonSerializableOptions, "IgnoreInaccessibleMembers">> |
There was a problem hiding this comment.
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.
| * | ||
| * 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. |
There was a problem hiding this comment.
I think we like avoiding enums anyway so seems like the right priority
There was a problem hiding this comment.
So that helper has that problem. Then we use the other helper - lines below to manage that.
JsonString<T>represents a string that has a typeTencoded within.If a value of type
Twere given toJSON.stringify(and is serializable), the string produced isJsonString<T>.That same string passed to
JSON.parse, will produce a value of typeJsonDeserialized<T>. (Value may not be of typeTbecause there may be loss during stringify.JsonDeserializedfilter is very accurate representation of the result.)JsonStringifyandJsonParseutilities are provided that output and take-inJsonStringstrings using regularJSON.stringifyandJSON.parse, respectively.JsonStringifyemploysJsonSerializabletype filter on input to reveal most serialization compatibility issues.Test cases added: