Skip to content
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

JSON serialization of Realm.Object & Realm.Collection (incl. polished TS declarations) #3044

Merged
merged 24 commits into from
Aug 14, 2020

Conversation

steffenagger
Copy link
Contributor

@steffenagger steffenagger commented Jul 8, 2020

JSON serialization of Realm.Object & Realm.Collection & a rework of TS declarations

This PR is a port of #3013 to the v10 branch.

These changes fixes #2997 & drastically simplifies realm/realm-studio#1312

Progress (subject to change)

TS declarations (used in integration-tests)

  • Limit input in create<T>(...) to match a model (using @nirinchev's ExtractPropertyNamesOfType©) notion of any implications this could introduce, is welcome!.
  • Allow input to contain Array<T> in RealmPartialModel<T> instead of a defined Realm.Collection<T> in create<T>(...).
  • Update objects<T>(...) to accommodate the underlying logic & different responses.
  • Allow extending Realm.Object for class models.

toJSON on Realm.Object & Realm.Collection (this includes Realm.List & Realm.Results)

  • Implement solution with object references (this introduces a cache to not return new objects).
  • Ensure TypeError: Converting circular structure to JSON is thrown for circular structures, not RangeError: Maximum call stack size exceeded.
  • Implement & expose replacer for circular structures. *

* I've included a combined key of the table-type & _objectId() as unique $refId in Realm.JsonSerializationReplacer, for a (visual) reference in serialized JSON. This is only used in case a primaryKey is not defined.

Integration Tests

  • Write test covering toJSON implementations for both interface/schema & class model usage.

Changelog

  • Update CHANGELOG.md

Copy link
Contributor

@kneth kneth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@blagoev blagoev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the PR is good. with some comments. Good job 👍

lib/extensions.js Show resolved Hide resolved
lib/extensions.js Show resolved Hide resolved
lib/extensions.js Outdated Show resolved Hide resolved
lib/extensions.js Outdated Show resolved Hide resolved
lib/extensions.js Show resolved Hide resolved
Copy link
Member

@kraenhansen kraenhansen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! This is a minor suggestion to only call toJSON when we hit one of our own Collection / Object types.

lib/extensions.js Outdated Show resolved Hide resolved
@kraenhansen kraenhansen self-assigned this Jul 10, 2020
@kraenhansen
Copy link
Member

I'll be moving this forward while Steffen is 🍹 🏖️

@kraenhansen kraenhansen force-pushed the kh/fixing-integration-tests branch from dad9080 to e896325 Compare July 10, 2020 14:35
@kraenhansen kraenhansen force-pushed the kh/fixing-integration-tests branch from e896325 to 4458651 Compare July 30, 2020 17:43
Base automatically changed from kh/fixing-integration-tests to v10 August 12, 2020 12:50
@steffenagger steffenagger merged commit 3dc53e9 into v10 Aug 14, 2020
@steffenagger steffenagger deleted the sa/v10/realm-tojson branch August 14, 2020 10:31
@hudon
Copy link

hudon commented Sep 24, 2020

correct me if I'm wrong, but I think this makes the type-checking too strict when you're updating a record using create(...UpdateMode.all) with a partial record, like:

interface Record {
  id: number;
  foo: number;
  bar: string;
}
const partOfModel: Partial<Record> = { id: 1, foo: 3 };
realm.create<Record>('record', partOfModel, UpdateMode.All);

TypeScript will throw an error saying Argument of type 'Partial<Record>' is not assignable to parameter of type 'RealmInsertionModel<Record>'

FYI @blagoev @steffenagger

@steffenagger
Copy link
Contributor Author

@hudon I suspect you're right - and thanks for the example.
The thing that springs to mind is if using UpdateMode.Never we would still like to require a full model / warn about missing props... I'll have a look!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants