-
Notifications
You must be signed in to change notification settings - Fork 540
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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
@jason-ha , for "JsonSerializable should eventually replace @fluidframework/datastore-definitions's Jsonable" , we should create a backlog item to track it, with details on which version it would be viable to do so. |
It would be a cool follow-up to have JsonString type that is type branded string that implies that if you parse it you'll get JsonDeserialized or something (not sure exactly how to leverage your types here). And then we write strongly-typed wrappers for I've prototyped this a few times, would be very useful in ContainerRuntime layer where we pass around stringified stuff all the time. |
Maybe I can FHL it. Something like |
Agreed. If the internal uses continue to look good, we could make a change for 3.0. (The Pages codebase may not be able to transition before - there was some cleanup needed when I made fixes to Jsonable a while back.) |
- remove extraneous test types - rename test value for accuracy - also comment corrections
Separate internal recursion detection use of AllowExactly filter control from user given so that user may specify `unknown`. A better approach would be to turn AllowExactly into a tuple. Experimentation as needed as in the past, tuple use appeared to cause other issues.
Support primitives intersected with classes to be serialized (`JsonSerializable`). (`JsonDeserialized` already had support.)
Update `AllowExactly` option to accept tuple of types that is more flexible than union as it allows `unknown` to be mixed with other types.
Now that AllowExactly is a tuple ReplacementMaker can be added to it directly instead of requiring a separate control property even when AllowExactly may contain `unknown`.
especially for those with `unknown` value types.
… for users Instead of some results displaying `FormDegenerate...` use direct formation to have `JsonTypeWith` show up explicitly.
I worked on this during FHL which found some interesting use cases (branded strings and explicit |
@@ -415,13 +481,80 @@ describe("JsonDeserialized", () => { | |||
assertIdenticalTypes(resultRead, objectWithNumberKey); | |||
}); | |||
|
|||
it("object with array of supported types (numbers) are preserved", () => { | |||
const resultRead = passThru(objectWithArrayOfNumbers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have generated the type name based on the variable name, but the fact that you typed out both gives me confidence for how thoroughly you've thought through these test cases!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of cases where the name is exactly what is intended to be covered. Then there are some like this one that is meant to handle a set of things but only tests a specific case. We only test array of numbers, but numbers is proxy for other supported things like booleans, strings, and supported objects.
All of the tests:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disclaimer: I did not review the 1000 lines of metaprogramming, but the breadth and depth of test cases gives me a lot of confidence. And as discussed, the risks of something bad happening in production here are low - if someone thinks they're serializing something but it's not happening right and the type allows it, they should have tests to cover the runtime behavior.
Add pair of type filters for JSON based serialization. `JsonSerializable<T>` produces type representing limitations of serializing `T`. Incompatible elements are transformed to `never` or `SerializationError*` types that original `T` is not assignable to. `JsonDeserialized<T>` produces type representing result of `T` being serialized and then deserialized. `JsonSerializable` should eventually replace `@fluidframework/datastore-definitions`'s `Jsonable`. That cannot done be currently as it would be a compile-time breaking change. [AB#6887](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/6887) ### Additional changes beyond Presence implementation - New support added for `unknown` as and exactly allowed option. - `AllowExactly` option changed to tuple in support. - Helper added to detect template literals - Helper added to detect index signatures - Support for branded primitives (`boolean`, `number`, and `string`) - Additional comments and corrections - Test coverage for arrays nested in objects ### Supporting Changes Add standard test infrastructure --------- Co-authored-by: Daniel Lehenbauer <[email protected]>
Add pair of type filters for JSON based serialization.
JsonSerializable<T>
produces type representing limitations of serializingT
. Incompatible elements are transformed tonever
orSerializationError*
types that originalT
is not assignable to.JsonDeserialized<T>
produces type representing result ofT
being serialized and then deserialized.JsonSerializable
should eventually replace@fluidframework/datastore-definitions
'sJsonable
. That cannot done be currently as it would be a compile-time breaking change.AB#6887
Additional changes beyond Presence implementation
unknown
as and exactly allowed option.AllowExactly
option changed to tuple in support.boolean
,number
, andstring
)Supporting Changes
Add standard test infrastructure