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

Add toJSON implementation and testing #4966

Merged
merged 15 commits into from
Oct 12, 2022
Merged

Add toJSON implementation and testing #4966

merged 15 commits into from
Oct 12, 2022

Conversation

gagik
Copy link
Contributor

@gagik gagik commented Sep 30, 2022

What, How & Why?

This is a breaking change in sense that it reintroduces toJSON functionality with the binding generator but removes JSONSerializerReplacer. Instead, 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 toJSON is therefore to generate valid, plain JavaScript objects with correct circular references so those can be used by the packages mentioned above. Current implementation supports Objects and Collections. It is nearly completely ported from pre-bindgen implementation, see this for objects and this for collections. Some changes were made to adapt both to the current state of the API and possibly improve performance by using a 2D map.

This PR is not complete and still needs feedback regarding some approaches and testing, see my review for details.

This closes #4949

☑️ ToDos

  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 📝 Update COMPATIBILITY.md
  • 🚦 Tests
  • 🔀 Executed flexible sync tests locally if modifying flexible sync
  • 📦 Updated internal package version in consuming package.jsons (if updating internal packages)
  • 📱 Check the React Native/other sample apps work if necessary
  • 📝 Public documentation PR created or is not necessary
  • [] 💥 Breaking label has been applied or is not necessary

If this PR adds or changes public API's:

  • typescript definitions file is updated
  • jsdoc files updated
  • Chrome debug API is updated if API is available on React Native

Copy link
Contributor Author

@gagik gagik left a comment

Choose a reason for hiding this comment

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

Some things I am not sure about

integration-tests/tests/src/tests/serialization.ts Outdated Show resolved Hide resolved
// Check that the serializable object is the same as the first related object.
// (this check only makes sense because of our structure)
expect(serializable).equals(serializable.related[0]);
const relatedObjects = serializable.related as Array<typeof serializable>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if using typeof serializable is okay here / in general, not too familiar with practices just yet.

packages/realm/src/Collection.ts Outdated Show resolved Hide resolved
packages/realm/src/Collection.ts Outdated Show resolved Hide resolved
packages/realm/src/Object.ts Outdated Show resolved Hide resolved
packages/realm/src/Object.ts Outdated Show resolved Hide resolved
packages/realm/src/Object.ts Outdated Show resolved Hide resolved
@gagik gagik marked this pull request as draft September 30, 2022 13:55
@gagik gagik changed the title WIP: Add toJSON implementation and testing Add toJSON implementation and testing Sep 30, 2022
Copy link
Contributor Author

@gagik gagik left a comment

Choose a reason for hiding this comment

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

Some things that seems worthy to note

integration-tests/tests/src/tests/serialization.ts Outdated Show resolved Hide resolved
packages/realm/src/Dictionary.ts Outdated Show resolved Hide resolved
packages/realm/src/Object.ts Outdated Show resolved Hide resolved
integration-tests/tests/package.json Outdated Show resolved Hide resolved
integration-tests/tests/src/tests/serialization.ts Outdated 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.

I have a few comments. This is very close to mergeable.

@gagik gagik marked this pull request as ready for review October 11, 2022 12:06
@gagik gagik merged commit 66bcdcd into bindgen Oct 12, 2022
@gagik gagik deleted the gagik/tojson branch October 12, 2022 12:34
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants