-
Notifications
You must be signed in to change notification settings - Fork 52
fix: recursive ToJSON #174
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
Conversation
johannes-lindgren
left a comment
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.
@endel, what do you think of this proposal? If you like it, would you rather include it in version 3?
What motivates me to open this PR is that I am creating a game with React, but the ToJSON from this library is insufficient. I was using my own version (and wrappers to toJSON()), and thought that others might be interested in a solution as well 🙂
There is a failing check from codeclimate which I don't know how to resolve.
| if (!deprecated[field] && this[field] !== null && typeof (this[field]) !== "undefined") { | ||
| obj[field] = (typeof (this[field]['toJSON']) === "function") | ||
| ? this[field]['toJSON']() | ||
| : this[`_${field}`]; |
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 don't understand this part: why is there an underscore?
src/Schema.ts
Outdated
| toJSON (): NonFunctionProps<{ | ||
| [field in keyof this]: ToJSON<this[field]> | ||
| }> { |
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 suspect this type is not entirely accurate, because there is a conditional that seems to omit null values. This hasn't been taken into account, but this isn't being taken into account in the current implementation anyways.
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.
Line 41 tests some types, but these types are never checked unless this file is moved into a subdirectory to the test folder.
|
Thank you for the PR @johannes-lindgren, I'm going to review this soon! |
|
Hi @johannes-lindgren, thanks again for the PR, sorry for taking long to review it... I'm just trying to test via Are you getting this error too? As a side note, this is what I'm currently using in some React projects to workaround the const roomState = useState<ReturnType<MyRoomState["toJSON"]>>(undefined); |
|
Hi @endel, sorry for the failing test. I could have sworn that I ran all the test locally, but apparently, I didn't 😉 I will have a look again; the TypeScript error appears in recursive types. |
|
@endel, here's an update: 0591a6d I fixed the issue with the recursive schemas, but now I am struggling with another type error in View TypeScript error messageThe error appears on the last line of this code: // Defines a generic schema
interface SchemaInterface extends Schema {
players: Map<string, string>;
items: string[];
}
// Implements the above interface
// MapSchema is compatible with Map
class SchemaInterfaceImpl extends Schema implements SchemaInterface {
players: MapSchema<string>;
items: ArraySchema<string>;
}
// Uses the schema interface
abstract class AbstractRoom<T extends SchemaInterface> { }
// Uses the schema implementation
class AbstractRoomImpl extends AbstractRoom<SchemaInterfaceImpl> { }The error stems from a mismatch of function parameter types in assign(
props: { [prop in NonFunctionPropNames<this>]?: this[prop] } | ToJSON<this>,
) {
Object.assign(this, props);
return this;
}I am relatively new to @colyseus/schema, so I don't understand how a "json-ified" version of the schema can be safely assigned? For example, if my schema expects the property |
|
I haven't dove too deep into the changes, but I think it can be easily done by just changing the I wrote a type utility specifically for solving this. type InferSchemaJSON<T> =
T extends ArraySchema<infer U>
? InferSchemaJSON<U>[]
: T extends CollectionSchema<infer U>
? InferSchemaJSON<U>[]
: T extends SetSchema<infer U>
? InferSchemaJSON<U>[]
: T extends MapSchema<infer U>
? { [key: string]: InferSchemaJSON<U> }
: T extends { toJSON: () => any }
? InferSchemaJSON<ReturnType<T["toJSON"]>>
: { [K in keyof T]: InferSchemaJSON<T[K]> };
export type GameRoomStateJSON = InferSchemaJSON<GameRoomState>; |
|
Thank you so much for the implementation idea @chungweileong94 🥳 Please let me know if you have any issues after upgrading. |
This pull request addresses two issues with the
ToJSONutility type:ToJSONutility type does not transform nested objects. This means that you cannot define and initialize your state with `useState<ToJSON>(() => ({ ... }))toJSONon ArraySchema, MapSchema, and CollectionSchema. The return type was implicitlyany. Fixes TypeScript: toJSON() return type on MapSchema / ArraySchema #162Regarding the first point, there were two major problems:
If you call
.toJson()on a schema that has a property that is not mapped, the property would not be transformed byToJsonEven if you call
.toJson()on a schema that has a property that is mapped, if the values in the map has non-primitive schemas, those will not be mapped either.which meant that you would get two type error if you tried to do
Implementation
ToJSONcontained many conditionals (one for MapSchema, Map, ArraySchema). Rather having all these conditionals , this PR changes the return type of thetoJSONfunctions on each class so that they themselves describe the type transformation that takes place.ToJSONgets changed so that it simply evaluates to the return type of thetoJSONfunction.Question:
ToJSONhad a conditional forMap, but this came after theMapSchemaconditional. SinceMapSchemaimplements Map, I think theMapconditional never gets evaluated, so I don't think the new implementation ofToJSONneeds to take this into consideration. Is this right, @endel?Tests
I added a few tests that tests the
ToJSONutility type. Since they are testing a utility type, these tests are checked by the type checker.For some reason which I don't understand, the type checking would not be performed on the tests if they were placed directly in the
testdirectory. As a workaround, I created atest/srcdirectory to which I moved these tests.