feat(client): JsonSerializable and JsonDeserialized #21725
Conversation
+ test objects with certain properties Note some new Deserialized tests are failing
+ test Replacement cases
+ cleanup error for required property with undefined
when all properties are supported.
for deserialization tests in place of class instances
by using homomorphic mapping with `as`
and misc formatting improvements
and detach expect type from input type T to avoid influencing inference of T
reorganize for documentation and add/correct some docs
⯅ @fluid-example/bundle-size-tests: +245 Bytes
Baseline commit: 6fd320c |
| * @system | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-namespace | ||
| export namespace InternalUtilityTypes { |
There was a problem hiding this comment.
Nit: Consider renaming to 'InternalTypes' for consistency with tree DDS?
import * as InternalTypes from "./internalTypes.js";
export {
/**
* Contains types used by the API, but which serve mechanical purposes and do not represent semantic concepts.
* They are used internally to implement API aspects, but are not intended for use by external consumers.
*/
InternalTypes,
};There was a problem hiding this comment.
I am expecting that we are going to end up with multiple namespaces in some packages for multiple internal things and the only thing that needs to be consistent (my opinion) is Internal prefix on the namespace. None of these should be imported from outside of the package (darn api-extractor); so, I think name that makes sense for internal description is good AND we should be able to arbitrarily change these names without it being considered a breaking change for customers.
BTW tree is also investigating use of explicit namespaces similar to what I used here. I tried the tree pattern show above, but api-extractor is unhappy about {@link } references when you rename like that.
renames and a rework for safe use
in core-interfaces squash of PR microsoft#21725
|
@CraigMacomber and/or @anthony-murphy, would you be able to find time to review these changes? (Per Daniel's suggestion as he has run out of time before vacation.) |
|
|
||
| /** | ||
| * Collection of utility types that are not intended to be used/imported | ||
| * directly outside of this package. |
There was a problem hiding this comment.
Might be worth including in this comment they may not follow semver, since I assume thats the intention (both to communicate it to devs working on/using/reviewing these types but also to customers)
| /** | ||
| * Sentinel type for use when marking points of recursion (in a recursive type). | ||
| * Type is expected to be unique, though no lengths is taken to ensure that. | ||
| * | ||
| * @beta | ||
| * @system | ||
| */ | ||
| export interface RecursionMarker { | ||
| "recursion here": "recursion here"; | ||
| } |
There was a problem hiding this comment.
It's possible for this to collide with regular data. Picking something faster to compare and collision resistant seems like it would be a good idea. unique symbols seem ideal for that.
There was a problem hiding this comment.
I am open to suggestions. I had mixed luck trying to use symbols in here. So, I didn't over think it.
| /** | ||
| * A type that is managed by custom deserialization logic (beyond JSON.parse). | ||
| * | ||
| * The default value is `never`. | ||
| */ | ||
| Replaced: unknown; |
There was a problem hiding this comment.
This is a bit confusing. This seems like a runtime set of options (based on it not being a generic type to take in the parameter), but never is a compile time only thing, and this value isn't optional so how does it have a default anyway?
There was a problem hiding this comment.
Breaking type out like this is the guidance for our docs. It is only to be used with JsonDeserialized type param and that is where the default exists. Looked more reasonable on the doc site.
| * | ||
| * @beta | ||
| */ | ||
| export type JsonDeserialized< |
There was a problem hiding this comment.
Given this is beta and not internal, I assume there is some public place we intend to use this API? Jsonable is currently @legacy @alpha, so this is more public than needed to replace that.
MatrixItem and ISharedSummaryBlock seem to be the only exported APIs using Jsonable. Is this intended for them or maybe some future use-case?
There was a problem hiding this comment.
Jsonable is @legacy so this needs to be @public to be able to replace it. (Actually, it is JsonSerializable that will replace Jsonable. There is no deserialized counterpart in the existing stuff.)
Already noted in the description. :)
| * Deserialized JSON never contains `undefined`, `symbol`, or function values. | ||
| * Object properties with values of those types are absent. So properties | ||
| * become optional (when there are other supported types in union) or are | ||
| * removed (when nothing else in union is supported). | ||
| * In an array, such values are replaced with `null`. |
There was a problem hiding this comment.
When using tools like git which are line based, personally I prefer more semantic line breaks over arbitrary ones mainly
Starting sentences on new lines. Also, I don't think starting a sentence with a coordinating conjunction like "so" like that is correct. either than period should become a comma, or we can use a colon here (which I think makes the most sense):
| * Deserialized JSON never contains `undefined`, `symbol`, or function values. | |
| * Object properties with values of those types are absent. So properties | |
| * become optional (when there are other supported types in union) or are | |
| * removed (when nothing else in union is supported). | |
| * In an array, such values are replaced with `null`. | |
| * Deserialized JSON never contains `undefined`, `symbol`, or function values. | |
| * Object properties which had those values before encoding are omitted by JSON.stringify: | |
| * Therefor in this type properties become optional (when there are other supported types in union) | |
| * or are removed (when nothing else in union is supported). | |
| * In an array, such values are replaced with `null`. |
| * `bigint` valued properties are simply removed as serialization attempts | ||
| * will throw. |
There was a problem hiding this comment.
There are a lot of things this doc comment does not cover, despite it seeming to make an attempt to be complete, so I'll mention them here:
- Its perfectly possible to losslessly encode bigints into and from JSON. JSON allows arbitrary numeric precision. This doc comment makes no mention of JSON.strinify, so applying its limitation here seems arbitrary. There even is https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/rawJSON which makes it possible to to losslesslyencode bigints using JSON.stringify ( https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON#using_json_numbers )
- No mention of what subset of properties are considered at runtime. Typically, this is enumerable own properties. I thing the section on setters/getters is simply wrong. I'm not sure what you mean by "backed by a function": setters and getters always use functions. Assuming we are talking about JSON.strinify and similar functions what actually matters is if the property is an enumerable own property. Setter don't matter and getters are not special really as they are included when enumerable own. Since TypeScript does not capture ownness or enumerableness of properties in its type system, and most serializers including our stringify and toJson rely on them think this needs to be called out explicitly.
- No mention of the JSON limits on strings or numbers. Strings with unmatched utf16 surrogate pairs will not round trip, nor will NaN or infinities.
- You mention recursive types, but not recursive values
Generally the existing doc comment on Jsonable seems to cover a lot of details which are missing here (ex: the special number cases, assumptions about enumerability of properties, issues with ArrayLike, recursive object issues. If this is intending to replace that API, it seems like its docs should be at least as complete.
There was a problem hiding this comment.
Maybe this should link JsonSerializable's docs to cover all these cases, and document (at least in a private remarks) why we need both and how they differ?
| : /* non-object => T as is */ T; | ||
|
|
||
| /** | ||
| * Test for non-public properties (class instance type) |
There was a problem hiding this comment.
I played with this a bit. Its surprisingly robust. Neat utility.
| * Test for non-public properties (class instance type) | |
| * Test for non-public properties (which can only exist on class instance types). | |
| * Returns true if `T` deeply may contain (via non function members) a private or protected field. | |
| * @remarks | |
| * This does not distribute over unions: `ReplaceRecursionWith<A> | ReplaceRecursionWith<B>` will produce Boolean when `ReplaceRecursionWith<A | B>` produces true; |
I did however find at least one edge case it doesn't handle: classes which extend Function return false even if they have private members. I didn't try out recursive cases.
Might be worth calling out that this has no impact on properties which are present on objects, but not included in the type. That may be obvious, but with the ways this is used, that limitation is likely to impact people. If I implement an interface with a class, or up cast a more specific type to a more general interface before it goes into the JsonSeralizable logic, it all fails to detect the omitted members despite them still impacting runtime. This limitation seems more likely to cause issues most of the things it does detect, which makes me wonder if this whole mechanism is really worth all the complexity as there is no way to fix that issue.
| * | ||
| * @beta | ||
| */ | ||
| export type JsonDeserialized< |
There was a problem hiding this comment.
I see reasonable (but in my opinion not particularly well motivated given the complexity and limitations) use-cases for JsonSserialized, but JsonDeserialized doesn't have a usecase I can find other than casting unvalidated deserialized data to a type it might not actually be compatible with. While I'm ok with customers doing such casts when they think its safe, them doing so does not require this type since they can just cast to their actual type.
Having us use this inside a library seems like its encapsulating a possibly incorrect cast which there is no possible way to validate into our framework, which seems like a bad idea, especially if we expose it publicly. We might have some internal use-cases for this, but I don't think we should use it in a public API, so maybe this specific type should be marked internal instead of beta?
There was a problem hiding this comment.
In the world of signals, it is all very badly typed and there is little validation. It is kind of impressive but also makes me feel confident when I add a new use that there aren't weird things making use of this for type improvements.
There are definitely risk-benefit tradeoffs being made.
As discussed in Teams chat, I think there is space to add some validation option or at least note where things are not otherwise validated.
| * | ||
| * @beta | ||
| */ | ||
| export type JsonSerializable< |
There was a problem hiding this comment.
Given we don't have actual users for this yet, is there a reason for this to skip the alpha stage straight to beta? Seems like this could either start as internal or alpha, and get promoted to beta when the libraries that depend on it need to be marked beta?
I'm generally in favor of not stabilizing things any more than we have a reason to maximize our flexibility and API readability (unused things in API docs make finding things you need harder).
There was a problem hiding this comment.
I believe @vladsud has a use and I want it for experimental DIS that is very definitely getting promoted. And I started this a while back. So when I made @alpha available I didn't try to demote it. I certainly can.
| * result can be more informative than just the type name. | ||
| * | ||
| * @beta | ||
| * @system |
There was a problem hiding this comment.
do we document @system user facing anywhere yet?
There was a problem hiding this comment.
We need to add support to the website. @Josmithr, want to give it a go? :) I did add notes to our wiki: https://github.com/microsoft/FluidFramework/wiki/TSDoc-Guidelines#system
Pair of type filters for JSON based serialization.
JsonSerializable<T>produces type representing limitations of serializingT. Incompatible elements are transformed toneverorSerializationError*types that orignalTis not assignable to.JsonDeserialized<T>produces type representing result ofTbeing serialized and then deserialized.JsonSerializableshould eventually replace@fluidframework/datastore-definitions'sJsonable. That cannot done be currently as it would be a compile-time breaking change.AB#6887