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

@realm/react key-path filtering on useQuery and useObject #6360

Merged
merged 27 commits into from
Feb 16, 2024

Conversation

kraenhansen
Copy link
Member

What, How & Why?

This closes #6286

☑️ ToDos

  • Figure out a way to document the overload of useQuery
  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 📝 Update COMPATIBILITY.md
  • 🚦 Tests
  • 📦 Updated internal package version in consuming package.jsons (if updating internal packages)
  • 📱 Check the React Native/other sample apps work if necessary
  • 💥 Breaking label has been applied or is not necessary

@kraenhansen kraenhansen self-assigned this Jan 9, 2024
@@ -93,6 +93,7 @@ type RealmContext = {
* ```
* @param type - The object type, depicted by a string or a class extending {@link Realm.Object}
* @param primaryKey - The primary key of the desired object which will be retrieved using {@link Realm.objectForPrimaryKey}
* @param keyPaths - Indicates a lower bound on the changes relevant for the hook. This is a lower bound, since if multiple hooks add listeners (each with their own `keyPaths`) the union of these key-paths will determine the changes that are considered relevant for all listeners registered on the object. In other words: A listener might fire and cause a re-render more than the key-paths specify, if other listeners with different key-paths are present.
Copy link
Member Author

Choose a reason for hiding this comment

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

I've added this here, but I still need to figure out a way to document the overload on useQuery.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we deprecate the previous overloads? Then you can just document the new way and mention the old as deprecated.

// We want the user of this hook to be able pass in the `query` function inline (without the need to `useCallback` on it)
// This means that the query function is unstable and will be a redefined on each render of the component where `useQuery` is used
// Therefore we use the `deps` array to memoize the query function internally, and only use the returned `queryCallback`
/* eslint-disable-next-line react-hooks/exhaustive-deps -- We want the user of this hook to be able pass in the `query` function inline (without the need to `useCallback` on it)
Copy link
Member Author

Choose a reason for hiding this comment

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

Turned this comment into an eslint-disable-next-line comment to silence the warning.

* Opens a test realm at a random temporary path.
* @returns The `realm` and a `write` function, which will wrap `realm.write` with an `act` and prepand a second `realm.write` to force notifications to trigger.
*/
export function createRealmTestContext(rootConfig: Configuration = {}): RealmTestContext {
Copy link
Member Author

Choose a reason for hiding this comment

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

I've added this helper, to make it easier to work with Realms in the tests.

Comment on lines 51 to 56
act(() => {
context.realm.write(callback);
// Starting another write transaction will force notifications to fire
context.realm.beginTransaction();
context.realm.cancelTransaction();
});
Copy link
Member Author

Choose a reason for hiding this comment

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

This simplifies the test code quite a bit, ensuring act is called and applying the workaround to force notifications to fire.

Copy link
Contributor

Choose a reason for hiding this comment

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

Were notifications not firing without that workaround?

Copy link
Member Author

@kraenhansen kraenhansen Jan 29, 2024

Choose a reason for hiding this comment

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

Were notifications not firing without that workaround?

Yes, but they only fire when the database advances, which happens asynchronously if not pressured by an explicit write transaction. This force the notifications to fire synchronously.
(I'll add that last word to the comment in the code).

@kraenhansen kraenhansen marked this pull request as draft January 9, 2024 20:42
@kraenhansen
Copy link
Member Author

I'm drafting this to get some early feedback, I might want to merge another PR updating our TypeScript version to > 5.0 to get support for @overload in jsdocs 🤔

Copy link

coveralls-official bot commented Jan 9, 2024

Coverage Status

coverage: 85.346%. remained the same
when pulling ed9774e on kh/realm-react-key-path-notifications
into 7954133 on main.

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.

Great updates here. I really like the helpers for the tests.
I guess the only question is, should we deprecate the old argument methods for useQuery? Would be great to see that marked in consumer code so that it was apparent one should refactor.

@kraenhansen
Copy link
Member Author

should we deprecate the old argument methods for useQuery?

I thought about it, but I'd rather not deprecate something, pointing to another API, that also won't be the preferred way long-term.
I hope something like #6284 is the future 🤞 Although we need to incorporate keyPaths in that proposal 🤔

};

/**
* Opens a test realm at a random temporary path.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Opens a test realm at a random temporary path.
* Opens a test realm at a randomized and temporary path.

@@ -93,6 +93,7 @@ type RealmContext = {
* ```
* @param type - The object type, depicted by a string or a class extending {@link Realm.Object}
* @param primaryKey - The primary key of the desired object which will be retrieved using {@link Realm.objectForPrimaryKey}
* @param keyPaths - Indicates a lower bound on the changes relevant for the hook. This is a lower bound, since if multiple hooks add listeners (each with their own `keyPaths`) the union of these key-paths will determine the changes that are considered relevant for all listeners registered on the object. In other words: A listener might fire and cause a re-render more than the key-paths specify, if other listeners with different key-paths are present.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you give an example?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea, an @example could be nice. And if so, the same example could be added to the docs in the realm package part as well.

@@ -58,12 +63,16 @@ export function createUseObject(useRealm: () => Realm) {
// Ref: https://github.com/facebook/react/issues/14490
const cachedObjectRef = useRef<null | CachedObject>(null);

/* eslint-disable-next-line react-hooks/exhaustive-deps -- Memoizing the keyPaths to avoid renders */
const memoizedKeyPaths = useMemo(() => keyPaths, [JSON.stringify(keyPaths)]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hard not to read "memorized" 🤣

Copy link
Contributor

@elle-j elle-j left a comment

Choose a reason for hiding this comment

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

Great updates!

Comment on lines 51 to 56
act(() => {
context.realm.write(callback);
// Starting another write transaction will force notifications to fire
context.realm.beginTransaction();
context.realm.cancelTransaction();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Were notifications not firing without that workaround?

Comment on lines 53 to 54
context.openRealm();
const { realm } = context;
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the openRealm() on the context could return the realm as well? Then in places like this one you don't have to use context.realm or context.useRealm().

@@ -111,4 +97,74 @@ describe("useQueryHook", () => {

expect(collection[99]).toBe(undefined);
});

it("can filter objects via a query argument", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of updating the ..via a query option and ..via a query argument test names to e.g. ..via an option argument and ..via separate type and callback arguments. I initially interperted the "query argument" part as testing for e.g. $0 etc in the query.

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the names 🤞

@@ -93,6 +93,7 @@ type RealmContext = {
* ```
* @param type - The object type, depicted by a string or a class extending {@link Realm.Object}
* @param primaryKey - The primary key of the desired object which will be retrieved using {@link Realm.objectForPrimaryKey}
* @param keyPaths - Indicates a lower bound on the changes relevant for the hook. This is a lower bound, since if multiple hooks add listeners (each with their own `keyPaths`) the union of these key-paths will determine the changes that are considered relevant for all listeners registered on the object. In other words: A listener might fire and cause a re-render more than the key-paths specify, if other listeners with different key-paths are present.
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea, an @example could be nice. And if so, the same example could be added to the docs in the realm package part as well.

Comment on lines 29 to 39
type QueryHookOptions<T> = {
type: string;
query?: QueryCallback<T>;
keyPaths?: string[];
};

type QueryHookClassBasedOptions<T> = {
type: RealmClassType<T>;
query?: QueryCallback<T>;
keyPaths?: string[];
};
Copy link
Contributor

@elle-j elle-j Jan 17, 2024

Choose a reason for hiding this comment

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

Just realized that we should probably allow keyPaths?: string | string[] rather than just string[] since we do that in the realm package. Same for useObject.

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the API to allow a single string too.

@kraenhansen
Copy link
Member Author

I think this just needs a rebase and the inline doc comments needs a final update.

@kraenhansen kraenhansen force-pushed the kh/realm-react-key-path-notifications branch from fb613d7 to 307dc1b Compare January 29, 2024 13:21
@cla-bot cla-bot bot added the cla: yes label Jan 29, 2024
@kraenhansen kraenhansen marked this pull request as ready for review January 29, 2024 15:13
@kraenhansen
Copy link
Member Author

I believe this is finally ready for review. Unfortunately the upgrade to TS 5+ didn't solve the issue of missing support for @overload in doc comments.

I've created TypeStrong/typedoc#2492 in hopes we could get typedoc support for this - in the meantime I would love ideas on how to document this.

@@ -82,6 +80,44 @@ type RealmContext = {
* @param deps - An array of dependencies that will be passed to {@link React.useMemo}
* @returns a collection of realm objects or an empty array
*/

/**
* @overload
Copy link
Contributor

@takameyer takameyer Jan 30, 2024

Choose a reason for hiding this comment

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

In lieu of difficulties with the "overload" flag, why not just simplify the documentation down to what the desired usage is?
The type system can still handle the previous usage. My 2 cents.

Copy link
Member Author

Choose a reason for hiding this comment

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

the desired usage is?

Sure - but I don't know if I actually know what that is 🤔 I guess we could focus on passing an object and use this as a fix for #6259 and eventually make this more convenient via #6284?

Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion, passing as an object is the desired usage. If I would have thought of it earlier, it would be the only way to do it.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we do that, we could also @deprecate the existing signature for useQuery?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think that's the way forward. We are still on version 0.x.y, so we can make some broader changes. I think in the end it makes things more extensible if we want to add more options to the hooks

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated the docs to focus solely on the signature where an options object is passed 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

The README should also be updated. Or perhaps we should just simplify the README to point to the docs page 🤔

Copy link
Member Author

@kraenhansen kraenhansen Jan 30, 2024

Choose a reason for hiding this comment

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

I'm contemplating adding an overload, passing just the type as first argument to avoid deprecation of that:

const cars = useQuery(Car);

feels cleaner than

const cars = useQuery({ type: Car });

Copy link
Contributor

@takameyer takameyer Jan 30, 2024

Choose a reason for hiding this comment

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

Yeah, that's cleaner, but it's not "that" bad.

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.

Looks great 👍🏼

//
////////////////////////////////////////////////////////////////////////////

import React, { Profiler, ProfilerOnRenderCallback } from "react";
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice usage of the Profiler here 👍🏼


export type UseObjectHook = {
<T>(type: string, primaryKey: T[keyof T], keyPaths?: string | string[]): (T & Realm.Object<T>) | null;
<T extends Realm.Object<any>>(type: { new (...args: any): T }, primaryKey: T[keyof T], keyPaths?: string | string[]): T | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Appears we have a lint failure here and elsewhere

Copy link
Contributor

@elle-j elle-j left a comment

Choose a reason for hiding this comment

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

Great addition, getting closer to letting users try this out 🚀

packages/realm-react/src/__tests__/useObjectHook.test.tsx Outdated Show resolved Hide resolved
packages/realm-react/src/__tests__/useObjectHook.test.tsx Outdated Show resolved Hide resolved
Comment on lines 107 to 113
// Update the age and don't expect a re-render
write(() => {
if (result.current) {
result.current.age = 5;
}
});
expect(renders).toHaveLength(2);
Copy link
Contributor

@elle-j elle-j Jan 31, 2024

Choose a reason for hiding this comment

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

If result.current is falsy (and we thereby don't update the age), then the test may still pass since there is no new rerender. You do check the result.current in the expect before write(), so perhaps that's enough, but we could e.g. add an else clause in the callback and throw an error to make sure we test that the update doesn't cause a rerender in this case. (There's one more occurrence of this I believe.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, this should probably just assert result.current instead 👍

@@ -69,20 +52,22 @@ const testDataSet = [
{ _id: 6, name: "Sadie", color: "gold", gender: "female", age: 5 },
];

describe("useQueryHook", () => {
describe("useQuery", () => {
const context = createRealmTestContext({ schema: [dogSchema] });
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar comment as before (could destructure this here instead if you prefer).

packages/realm-react/src/index.tsx Outdated Show resolved Hide resolved
packages/realm-react/src/index.tsx Outdated Show resolved Hide resolved
Comment on lines 29 to 36
export type UseObjectHook = {
<T>(type: string, primaryKey: T[keyof T], keyPaths?: string | string[]): (T & Realm.Object<T>) | null;
<T extends AnyRealmObject>(
type: { new (...args: any): T },
primaryKey: T[keyof T],
keyPaths?: string | string[],
): T | null;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're now deprecating the usage of separate arguments for useQuery() in favor of an options arg, it introduces some inconsistency between that API and that of useObject(). Even though useObject() doesn't have the ESLint issue regarding dependency lists, I think we should consider allowing an options argument for that hook as well, especially since the third arg keyPaths has been introduced.

I'd be interested in hearing your thoughts on this @kraenhansen @takameyer .

Copy link
Contributor

Choose a reason for hiding this comment

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

Good consideration. I would be in favor of using an options object for useObject().

Copy link
Member Author

Choose a reason for hiding this comment

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

True -- that would probably be more consistent and read better for some users 👍

Copy link
Member Author

@kraenhansen kraenhansen Jan 31, 2024

Choose a reason for hiding this comment

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

I've pushed an update including that 👍

Copy link
Contributor

@elle-j elle-j Jan 31, 2024

Choose a reason for hiding this comment

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

Yeah I think so too. If updated in this PR, remember potential README and docs updates as well (saw your last comment now) 👍

packages/realm-react/src/useQuery.tsx Outdated Show resolved Hide resolved
@kraenhansen
Copy link
Member Author

kraenhansen commented Jan 31, 2024

To get the typedocs for these overloads to shine, I think I'll need to sink a bit more time into the way we export the functions from the default context in our index.ts 🤔

@kraenhansen kraenhansen marked this pull request as draft January 31, 2024 13:01
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. Does this need to be in draft? Maybe just a follow up PR for the documentation pieces.

packages/realm-react/CHANGELOG.md Outdated Show resolved Hide resolved
@kraenhansen
Copy link
Member Author

Does this need to be in draft? Maybe just a follow up PR for the documentation pieces.

I think you're right. I should probably just follow up with a PR to improve that documentation as this doesn't decrease the DX in and of itself.

@kraenhansen kraenhansen marked this pull request as ready for review February 16, 2024 15:34
@kraenhansen kraenhansen merged commit b54d7f6 into main Feb 16, 2024
9 checks passed
@kraenhansen kraenhansen deleted the kh/realm-react-key-path-notifications branch February 16, 2024 15:34
@kraenhansen
Copy link
Member Author

I've merged this as is and will follow up with updates to the docs of this once I get the docs for the entire package refactored a bit 👍

bimusiek pushed a commit to bimusiek/realm-js that referenced this pull request Mar 14, 2024
…m#6360)

* Seperated types from hook functions

* Adding a helper to generate a random realm path

* Use eslint-disable-next-line comment to disable a warning

* Adding a profileHook utility on-top-of renderHook

* Adding overloads to useQuery

* Adding failing tests

* Refactored tests into using a "realm test context"

* Passing key-paths through useQuery into the SDK's addListener method

* Reusing randomRealmPath

* Made useObject tests use createRealmTestContext

* adding a useObject test for re-render on object creation

* Implement keyPaths on useObject

* Incorporating feedback

* Allow passing a string as "keyPaths"

* Exporting types

* Adding @overload doc comments

* Focusing docs on the desired call-pattern

* Ran lint --fix

* Adding a AnyRealmObject utility type

* Using the `options` overload in the readme

* Adding a non-deprecated single argument overload

* Apply suggestions from code review

Co-authored-by: LJ <[email protected]>

* Using asserts instead of if-statements

* Moved RealmClassType, AnyRealmObject and isClassModelConstructor into helpers

* Implemented options object overload on useObject hook

* Adding a note to the changelog

* Update packages/realm-react/CHANGELOG.md

Co-authored-by: Andrew Meyer <[email protected]>

---------

Co-authored-by: LJ <[email protected]>
Co-authored-by: Andrew Meyer <[email protected]>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 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.

@realm/react Expose keypath filtering in useQuery and useObject
4 participants