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

Fix toJSON behaviour for Dictionary on JSC pre-v11 (fixes #4658) #4674

Merged
merged 2 commits into from
Jun 27, 2022

Conversation

tomduncalf
Copy link
Contributor

@tomduncalf tomduncalf commented Jun 24, 2022

What, How & Why?

This fixes the behaviour of toJSON on objects with Dictionary fields on JSC, reported in #4658.

Before this fix, Dictionary fields would incorrectly not be converted to JSON, resulting in an exception if a field of the Dictionary of the JSONified object was modified, as it would try to write to the Realm outside of a write transaction.

The root cause of this is similar to #4541, in that Proxy objects do not seem to be properly "transparent" on JSC – in this case, instanceof Realm.Dictionary would return false for a proxied Dictionary, whereas it should return true (as JS Proxy objects are supposed to be totally transparent). This should be fixed with JSI/Hermes.

I searched the codebase for anywhere else that might be affected by this bug and couldn't see anywhere.

☑️ ToDos

  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 🚦 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

@tomduncalf tomduncalf force-pushed the td/fix-jsc-tojson-dictionary branch from 4176095 to 40b58d5 Compare June 24, 2022 12:17
@takameyer
Copy link
Contributor

@tomduncalf Looks like we need to publish @realm/common in order for the tests to run

@tomduncalf
Copy link
Contributor Author

Yep, it's a slightly annoying set up @takameyer (will be fixed if realm becomes its own package) – I usually wait for the code to be approved before publishing it, then rerun with tests, as I'm not sure how easy unpublishing is if there's a change requested

@takameyer
Copy link
Contributor

@tomduncalf oh yeah...that is a bit annoying. And the change by itself in a PR doesn't really bring much, as it's very dependent on the other code being written. We will just have to wait for that wonderful world where realm is also a package.

Copy link
Contributor

@takameyer takameyer left a comment

Choose a reason for hiding this comment

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

LGTM

@tomduncalf tomduncalf closed this Jun 24, 2022
@tomduncalf tomduncalf reopened this Jun 24, 2022
@tomduncalf tomduncalf merged commit 419d7ca into master Jun 27, 2022
@tomduncalf tomduncalf deleted the td/fix-jsc-tojson-dictionary branch June 27, 2022 08:52
@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.

None yet

3 participants