Fix #4513

[mfbx9da4]

What, How & Why?

Fixes #4513 and #4913

[cla-bot]

Realm welcomes all contributions! The only requirement we have is that, like many other projects, we need to have a Contributor License Agreement (CLA) in place before we can accept any external code. Our own CLA is a modified version of the Apache Software Foundation’s CLA. Our records show that CLA has not been signed by @mfbx9da4. Please submit your CLA electronically using our Google form so we can accept your submissions. After signing the CLA you can recheck this PR with a @cla-bot check comment. The GitHub usernames you file there will need to match that of your Pull Requests. If you have any questions or cannot file the CLA electronically, make a comment here and we will be happy to help you out.

[mfbx9da4]

I didn't spend too long on this so might need some tweaking or cleaning up? @takameyer

This won't fix the fact that:

Using the same object via useObject in two different components will return different references

(as mentioned in #4513).

I imagine the code will have to change significantly to solve these latter two issues so not sure this commit is super useful.

[takameyer]

@mfbx9da4 Thanks for the PR!

Slightly different take than my idea:

return function useObject<T>(type: string | { new (): T }, primaryKey: PrimaryKey): (T & Realm.Object) | null {
    const realm = useRealm();

    // Create a forceRerender function for the cachedObject to use as its updateCallback, so that
    // the cachedObject can force the component using this hook to re-render when a change occurs.
    const [version, forceRerender] = useReducer((x) => x + 1, 0);

    // Wrap the cachedObject in useMemo, so we only replace it with a new instance if `primaryKey` or `type` change
    const { object, tearDown } = useMemo(
      // TODO: There will be an upcoming breaking change that makes objectForPrimaryKey return null
      // When this is implemented, remove `?? null`
      () =>
        createCachedObject({
          object: realm.objectForPrimaryKey(type, primaryKey) ?? null,
          updateCallback: forceRerender,
        }),
      [type, realm, primaryKey],
    );

    // Invoke the tearDown of the cachedObject when useObject is unmounted
    useEffect(() => {
      return tearDown;
    }, [tearDown]);

    return useMemo(() => {
      // If the object has been deleted or doesn't exist for the given primary key, just return null
      if (object === null || object?.isValid() === false) {
        return null;
      } else {
        // Wrap object in a proxy to update the reference on rerender ( should only rerender when something has changed )
        return new Proxy(object, {});
      }
      // We need version as a dependency in order to trigger a new object reference
    }, [version, object]);
  };

We will need to write a test case for this and the other issues in order to avoid regressions.

[mfbx9da4]

Yeah cool! I like your way of doing it. It would be cleaner IMO if the cached object API was tweaked though

function useObject<T>(type: string | { new (): T }, primaryKey: PrimaryKey): (T & Realm.Object) | null {
  const realm = useRealm()

  const cachedObject = useMemo<(T & Realm.Object) | null>(
    () => createCachedObject(realm, type, primaryKey),
    [realm, type, primaryKey]
  )
  const [version, setVersion] = useState(0)

  useEffect(() => {
    return cachedObject?.subscribe(() => setVersion(x => x + 1))
  }, [cachedObject])

  return useMemo(() => cachedObject.object ? new Proxy(cachedObject.object : null, {}), [cachedObject, version])
}

[takameyer]

Looks like we need to rebase and write a regression test. If you got some time to do that, we would be very grateful 🙂
Otherwise it may be some time before I get to it.

[takameyer]

Adding @tomduncalf and @kraenhansen to the review as well

[takameyer]

The code looks great, but I would like to see a regression test in both useQueryRender and useObjectRender that verifies that this fix works. Functionally it should trigger a re-render on the parent component (using useState) and verify that the children were not re-rendered.

It also appears this needs to rebase with master.

And please add an entry to the packages/realm-react/CHANGELOG.md 🙂

[takameyer]

I've squashed the current changes into a single commit and am continuing this PR on my own here:
#4958