fix(toJSON): get the generic type T for toJSON response #6789
Conversation
|
Realm welcomes all contributions! The only requirement we have is that, like many other projects, we need to have a Contributor License Agreement (CLA) in place before we can accept any external code. Our own CLA is a modified version of the Apache Software Foundation’s CLA. Our records show that CLA has not been signed by @vinibgoulart. Please submit your CLA electronically using our Google form so we can accept your submissions. After signing the CLA you can recheck this PR with a |
|
Realm welcomes all contributions! The only requirement we have is that, like many other projects, we need to have a Contributor License Agreement (CLA) in place before we can accept any external code. Our own CLA is a modified version of the Apache Software Foundation’s CLA. Our records show that CLA has not been signed by @vinibgoulart. Please submit your CLA electronically using our Google form so we can accept your submissions. After signing the CLA you can recheck this PR with a |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
packages/realm/src/Object.ts
Outdated
| @@ -382,17 +382,17 @@ export class RealmObject<T = DefaultObject, RequiredProperties extends keyof Omi | |||
| * and [flatted](https://www.npmjs.com/package/flatted) to stringify Realm entities that have circular structures. | |||
| * @returns A plain object. | |||
| */ | |||
| toJSON(_?: string, cache?: unknown): DefaultObject; | |||
| toJSON(_?: string, cache?: unknown): T | Partial<T>; | |||
There was a problem hiding this comment.
Why the Partial<T>?
Also - let's assume the model class has a property of type Realm.List. This is expected to return an array of values through toJSON, in which case using the T directly would be incorrect. We need a mapping, similar to the one we do in src/Unmanaged.ts (used by the Realm#create method).
There was a problem hiding this comment.
You're right, we need to mapping the array case. I'll check how can I improve it.
About the partial, when we are creating and mapping the values to create the object we have a partial of T, and we save in the cache, if some we have some break in the flow the cache will still be there (I assume) and the fn will return the partial of T, this is the mutabillity problem. Should I consider that we will never break and always return a complete object?
There was a problem hiding this comment.
we will never break and always return a complete object?
I not 100% sure I follow - please help me understand if I don't. What I think you're referring to is the fact that the object graph might contain cycles.
As I understand the toJSON semantics, it needs to transform objects into JSON serializable values, which might contain cycles and that's exactly what the implementation tries to do. I.e. if two objects refer to each other the cache would be used to detect the cycle and the resulting object would be two plain objects cyclicly referring to each other (as expected).
There was a problem hiding this comment.
Yes, on second thought, maybe we shouldn't lead the return as Partial<T>
There was a problem hiding this comment.
I think should return OmittedRealmTypes<T>
In document https://www.mongodb.com/docs/atlas/device-sdks/sdk/react-native/model-data/define-a-realm-object-model/
class Book extends Realm.Object<Book> {
name!: string;
price?: number;
static schema: ObjectSchema = {
name: 'Book',
properties: {
name: {type: 'string', indexed: true},
price: 'int?',
},
};
}T is Book (extends Realm.Object) so T has Realm.Object's methods, properties
but toJSON() remove those methods, properties like OmittedRealmTypes<T>
I patched it locally to toJSON(_?: string, cache?: unknown): OmittedRealmTypes<T>; and didn't encounter any issues while using it.
packages/realm/src/Object.ts
Outdated
| toJSON(_?: string, cache?: unknown): OmittedRealmTypes<T | Unmanaged<T>>; | ||
| /** @internal */ | ||
| toJSON(_?: string, cache = new JSONCacheMap<T>()): OmittedRealmTypes<T> { | ||
| toJSON(_?: string, cache = new JSONCacheMap<T>()): OmittedRealmTypes<T | Unmanaged<T>> { |
There was a problem hiding this comment.
Thank you for your commit.
Unmanaged<T> will remap all properties to optional.
If a property is not optional in its Realm.Object schema, instances created by that Object will certainly have that property and it will not be optional.
Will this cause the object returned by toJSON() to need to be remapped with type definitions in order to be used correctly?
Maybe OmittedRealmTypes<T> is sufficient?
What, How & Why?
Fixing the toJSON generic type return.
The
Tgeneric defined in the class instance already is setted asDefaultObjectif you doesnt send a generic, so this pr just use this type fortoJSONfunction return type.closes #6728
closes #5165
☑️ ToDos
Compatibilitylabel is updated or copied from previous entryCOMPATIBILITY.mdpackage.jsons (if updating internal packages)Breakinglabel has been applied or is not necessary@realm/devdocsif documentation changes are needed