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 a workaround for React Native Proxy objects pre-JSI not working properly with JSC. #4541

Merged
merged 4 commits into from
May 26, 2022

Conversation

tomduncalf
Copy link
Contributor

@tomduncalf tomduncalf commented Apr 29, 2022

What, How & Why?

As described in #4507 (comment), React Native Proxy objects which do not use JSI do not seem to be compatible with JSC – it doesn't "unwrap" them properly so can't see the wrapped type.

This means that you cannot pass a result from useQuery in @realm/react to the subscription set mutation functions, as it is wrapped in a Proxy.

This workaround stores the unproxied Results as a non-enumerable field on the proxied object, so that we can check for its presence and pass that to C++ instead of the proxied version when calling subscription set mutation functions.

Note that this problem seems to go away with JSI in v11 (regardless of whether Hermes is being used) so another option is that we just accept the issue for now, and wait until v11 becomes master

This closes #4507

☑️ ToDos

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

@cla-bot cla-bot bot added the cla: yes label Apr 29, 2022
@tomduncalf tomduncalf force-pushed the td/fix-usequery-type-with-subs branch from b891394 to 3f29ae0 Compare April 29, 2022 14:23
@tomduncalf tomduncalf force-pushed the td/fix-usequery-type-with-subs branch 5 times, most recently from 284e762 to 621a89d Compare April 29, 2022 15:10
Copy link
Member

@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.

My only comment is about naming of a constant and it should not block merging this PR.

// work fine, by reverting PR #4541.
const instanceMethods = (realmConstructor) => ({
add(query, options) {
return this._add(query[symbols.REALM_REACT_PROXIED_OBJECT] || query, options);
Copy link
Member

Choose a reason for hiding this comment

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

I have some reservations about REALM_REACT_PROXIED_OBJECT - it is leaking one package to another and in the opposite direction of the dependency. Could we find a neutral name?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps ORIGINAL_REALM_COLLECTION

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with PROXY_TARGET as per Kræn's suggestion

@tomduncalf
Copy link
Contributor Author

tomduncalf commented May 3, 2022

Closing in favour of waiting for v11 to be released which uses JSI and will fix the fundamental Proxy issue. Workaround for now is to pass in the results directly, e.g.

const people = useQuery(realm.objects('Person'));
// ...
realm.subscriptions.update(subs => {
  subs.add(people); // this will error
});

becomes

const results = realm.objects('Person');
const people = useQuery(people);
// ...
realm.subscriptions.update(subs => {
  subs.add(results); // no error as we are passing in the Realm.Results instance directly
});

@tomduncalf tomduncalf closed this May 3, 2022
@tomduncalf tomduncalf deleted the td/fix-usequery-type-with-subs branch May 3, 2022 10:11
@tomduncalf tomduncalf restored the td/fix-usequery-type-with-subs branch May 5, 2022 13:31
@tomduncalf tomduncalf reopened this May 5, 2022
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.

Seems like a very good way to getting around this issue. I would, however, like to see a test for this so that we can ensure it still works after v11 when we remove this code.

// work fine, by reverting PR #4541.
const instanceMethods = (realmConstructor) => ({
add(query, options) {
return this._add(query[symbols.REALM_REACT_PROXIED_OBJECT] || query, options);
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps ORIGINAL_REALM_COLLECTION


remove(query) {
return this._remove(query[symbols.REALM_REACT_PROXIED_OBJECT] || query);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems we are missing some linter fixes in this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 just minor suggestions.

Tom Duncalf added 3 commits May 25, 2022 14:42
… properly with JSC.

This change can be reverted once we go to v11. Fixes #4507
@tomduncalf tomduncalf force-pushed the td/fix-usequery-type-with-subs branch from 621a89d to 950da15 Compare May 25, 2022 14:13
@tomduncalf tomduncalf force-pushed the td/fix-usequery-type-with-subs branch from 950da15 to fb7983e Compare May 25, 2022 14:16
@tomduncalf
Copy link
Contributor Author

Seems like a very good way to getting around this issue. I would, however, like to see a test for this so that we can ensure it still works after v11 when we remove this code.

@takameyer I like the idea of adding a test but I am struggling to think of a good way to do it, because in order to get access to realm.subscriptions, you need to have a valid sync config, which means you need a Realm app to test against. This would mean we'd need to do one of:

  1. Make the Realm React tests support working with the app importer and move some of the Mocha hooks etc. used to interact with it from integration-tests into realm/common. This would be the right approach but feels like a lot of work for one test for a temporary(ish) test.
  2. Make the integration tests somehow test useQuery when running in React Native. Because it's a hook, this might mean setting up a whole React app to test in. Again, a lot of work and would also be breaking the separation of concerns in a weird way possibly (not sure if the main integration tests should be testing Realm React, although you could argue it's appropriate)
  3. Add some way of exposing subscriptions without a valid sync session for testing purposes, which feels quite hacky

If you have other ideas let me know though as I agree it would be good, but I'm not sure if it's worth a fairly major bit of work to achieve. I might be missing something easier. As is, we'll have to remember to test it again with v11 which I agree is not ideal!

@tomduncalf tomduncalf merged commit 34a8807 into master May 26, 2022
@tomduncalf tomduncalf deleted the td/fix-usequery-type-with-subs branch May 26, 2022 12:00
@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.

the result from useQuery is not usable in adding subscriptions.
4 participants