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
SharedTree: ChangeEncoder.decodeChange() should use 'Jsonable<T>' #11558
Conversation
| @@ -4,9 +4,11 @@ | |||
| */ | |||
|
|
|||
| import { bufferToString, IsoBuffer } from "@fluidframework/common-utils"; | |||
| import { Jsonable } from "@fluidframework/datastore-definitions"; | |||
|
|
|||
| export abstract class ChangeEncoder<TChange> { | |||
| public abstract encodeForJson(formatVersion: number, change: TChange): JsonCompatible; | |||
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'm curious about naming asymmetry between 'encodeForJson' and 'decodeJson'. Was that intentional?
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.
@CraigMacomber actually wrote this file and picked these names, but the names seem fairly clear to me. I wouldn't object to something like encodeAsJsonable and decodeFromJsonable.
|
@alex-pardes - Motivation for PR is to pass on knowledge of Jsonable, which isn't easily discoverable. |
| export abstract class ChangeEncoder<TChange> { | ||
| public abstract encodeForJson(formatVersion: number, change: TChange): JsonCompatible; |
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 think this PR would be much more valuable if we can delete JsonCompatible by doing something like this:
| export abstract class ChangeEncoder<TChange> { | |
| public abstract encodeForJson(formatVersion: number, change: TChange): JsonCompatible; | |
| export abstract class ChangeEncoder<TChange, Jsonable<TEncoded>> { | |
| public abstract encodeForJson(formatVersion: number, change: TChange): TEncoded; |
| @@ -4,9 +4,11 @@ | |||
| */ | |||
|
|
|||
| import { bufferToString, IsoBuffer } from "@fluidframework/common-utils"; | |||
| import { Jsonable } from "@fluidframework/datastore-definitions"; | |||
|
|
|||
| export abstract class ChangeEncoder<TChange> { | |||
| public abstract encodeForJson(formatVersion: number, change: TChange): JsonCompatible; | |||
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.
@CraigMacomber actually wrote this file and picked these names, but the names seem fairly clear to me. I wouldn't object to something like encodeAsJsonable and decodeFromJsonable.
The 'Jsonable' is another helper type that can catch some attempts to JSON serialize non-JSON serializable types.
It's typically used with type inference as follows:
See 'packages/runtime/datastore-definitions/src/jsonable.ts' for details.