Improve toJSON functionality and remove JSONSerializerReplacer #4997
Conversation
gagik
force-pushed
the
gagik/removejsonreplacer
branch
from
e342b91 to
6e481d8
Compare
| } | ||
|
|
||
| const Obj& obj = realm_object->obj(); | ||
| auto table_key = obj.get_table()->get_key(); |
There was a problem hiding this comment.
This seems like something to address in core: there is a private get_table_key() method which is just a shortcut to this. I think it'd be good to make that method public since I imagine this would be nice to use in contexts like this (and you can access it anyways so no reason not to).
There was a problem hiding this comment.
That's an interesting observation, and I suggest that you bring it up with the team working on the bindgen project (core's public API is part of the project).
CHANGELOG.md
Outdated
|
|
||
| ### Enhancements | ||
| * Small improvement to performance by caching JSI property String object [#4863](https://github.com/realm/realm-js/pull/4863) | ||
| * Small improvement to performance for `toJSON` which should make it useful for cases where a plain representations of Realm entities are needed, e.g. when inspecting them for debugging purposes through `console.log(realmObj.toJSON())`. |
There was a problem hiding this comment.
not sure if there is more to say / smth different to say.
| } | ||
|
|
||
| const Obj& obj = realm_object->obj(); | ||
| auto table_key = obj.get_table()->get_key(); |
There was a problem hiding this comment.
That's an interesting observation, and I suggest that you bring it up with the team working on the bindgen project (core's public API is part of the project).
| }, | ||
| ]; | ||
| const commonTests: Record<string, TestSetup> = { | ||
| //@ts-expect-error subject and serialized are set before tests run. |
There was a problem hiding this comment.
Can you share a bit more about this? It feels like this could be avoided.
There was a problem hiding this comment.
so, I can either make the subject and serialized fields as optional in which case I won't have this TS error here but then I will have to try to avoid all errors where I use subject but it could be undefined (even though it will never be undefined because it will be set before each test). This was my way of avoiding this but I can see how this is hacky, I can look into alternative
| }, | ||
| ]; | ||
| const commonTests: Record<string, TestSetup> = { | ||
| //@ts-expect-error subject and serialized are set before tests run. |
There was a problem hiding this comment.
so, I can either make the subject and serialized fields as optional in which case I won't have this TS error here but then I will have to try to avoid all errors where I use subject but it could be undefined (even though it will never be undefined because it will be set before each test). This was my way of avoiding this but I can see how this is hacky, I can look into alternative
gagik
force-pushed
the
gagik/removejsonreplacer
branch
from
1b67bca to
1caaed1
Compare
gagik
force-pushed
the
gagik/removejsonreplacer
branch
from
1caaed1 to
5ae0acf
Compare
gagik
force-pushed
the
gagik/removejsonreplacer
branch
from
06f920a to
0ff900c
Compare
What, How & Why?
This is a breaking change that removes
JSONSerializerReplacerand improves the currenttoJSONfunctionality. It also improves current tests, organizing them better and adding cases for dictionaries.With removal
JSONSerializerReplacer, users are advised to use external libraries such as flatted and @ungap/structured-clone if they need to deal with serialising objects with circular references into a JSON string.The expected functionality of
toJSONis therefore to generate valid, plain JavaScript objects with correct circular references so those can be used by the packages mentioned above.toJSONis implemented by Realm Object, Dictionary, and Collection.Related to #4966.
☑️ ToDos
Compatibilitylabel is updated or copied from previous entryCOMPATIBILITY.mdpackage.jsons (if updating internal packages)Breakinglabel has been applied or is not necessaryIf this PR adds or changes public API's: