aerogear / offix

GraphQL Offline Client and Server
https://offix.dev
Apache License 2.0
758 stars 45 forks source link

DataStore hooks for react #564

Closed Eunovo closed 4 years ago

Eunovo commented 4 years ago

Feature Request

We need to create generic hooks for Datastore. Currently, we are looking at passing the model as input to CRUD hooks, like this useAdd(model) or useDataStoreAdd(model)

@wtrocki @kingsleyzissou

Eunovo commented 4 years ago

There is the issue of typesafety in generic hooks too. If the hooks should be this generic, we might have to pass types. I'll need to investigate to confirm this though

wtrocki commented 4 years ago

I like useDataStore from my comment that were not linked here: https://github.com/aerogear/offix/pull/544#discussion_r447062249

Then you can just do ds.add(whatever). I found this simple and avoiding react shenanigans.

This means that you will simply have datastore object with all methods and access we need. Since we said that replication is separate thing we will need to have 2 separate hooks.

useReplicationEngine? and useDataStore?

kingsleyzissou commented 4 years ago

useDataStore sounds a lot better. The developer probably has no need to access the replication engine directly, they would be more interested in interacting with the datastore. At least that is my understanding of it.

Edit: I misread the last part of your comment, I thought that's the name you were suggesting for the hook. Will we need access to the replication engine through a hook? Would that be access to a method for manually triggering replication?

kingsleyzissou commented 4 years ago

https://github.com/aerogear/offix/issues/567

Kind of answers the edit to my last question.

Eunovo commented 4 years ago

Hooks Proposal @wtrocki @kingsleyzissou

We have strategies that we want to present to the user through hooks;

Any of the DB operations can be with or without subscriptions. We should have an option in query hook to enable subscriptions and accept subscription handlers to dictate how current data will be updated.

On the Edit side, we have;

We could use one hook with Model supplied that have CRUD methods that can accept appropriate options. See below

const { isLoading, error, data, ds } = useDataSync(Model, { forceDelta: true });
ds.add(input, { waitForReplication: true })
ds.query(predicate, { onNewData }) // here onNewData takes old data and new event data and returns the new data state
...other crud methods

The problem here is that all operations use the same response state i.e isLoading, error, data. Imagine what happens when you use multiple operations.

I prefer that each operation have it's own hook, like,

const { isLoading: isQueryLoading, error: queryError, data: queryResults, query } = useQuery(model, { forceDelta });
const { isLoading: isRequestLoading, error: saveError, data: savedData, add } = useAdd(model, { forceDelta }); // user doesn't need to force delta here 

add(input, options);
query(predicate, { ....options, onNewData });

Whichever method we choose, Model config can be set outside hooks. forceDelta is the only promise based operation and is the only one we need to add to hook options.

The reason I have not mentioned a useForceDelta hook is that every time the user wants to wait for delta before continuing, he/she would have to employ a useEffect hook. We can prevent this by simply adding a "forceDelta" option to CRUD hooks which will wait for delta to complete before returning data as I have shown above.

Also, I think user that forceDelta need to be alerted to the fact that there is no network connection. The operation the user is performing may critically require fresh data(hence the explicit request to forceDelta). I propose that as we set data, we also set the error state to a sort of OfflineError when the user is offline. It's the only way for the dev or user to know that delta failed.

Note: Any of the names used for methods, params, fields, hooks mentioned here can be changed if they are not considered satisfactory.

Eunovo commented 4 years ago

When it comes to queries, there are two ways we can handle it. For the first approach, the predicate is passed to the hook useQuery(model, predicate, options) For the second, the hook returns a query function that we can call with input predicate and options const { ..., query } = useQuery(model, { forceDelta });

Let's walk through usage for both approaches;

First Approach

Here predicate has to be derived from the current state and new data can only be fetched by changing state.

const { filter, setFilter } = useState({ ... }); // filter state render probably through some select boxes
const predicate = ... // some predicate gotten from filter
const { ... } = useQuery(model, predicate, ...);

Second Approach

Predicate doesn't have to be derived from the current state but to populate initial data, the dev has to use a useEffect hook to prevent infinite rerenders.

useEffect(() => {
    query(predicate, ...)
}, []);

I think we should go with the first approach:

Eunovo commented 4 years ago

Model Subscriptions do require devs to specify a useEffect hook with cleanup function. We can take that responsibility away by providing a hook like useSubscription(model, event, handler). This hook returns nothing. It just calls the handler when the target event occurs.

Note that there are two subscription scenarios, one described in initial proposal, updates query result when an event occurs. The one described here is not tied to any query.

kingsleyzissou commented 4 years ago
const { isLoading: isQueryLoading, error: queryError, data: queryResults, query } = useQuery(model, { forceDelta });
const { isLoading: isRequestLoading, error: saveError, data: savedData, add } = useAdd(model, { forceDelta }); // user doesn't need to force delta here 

If we go with something like this, instead of maybe having an isLoading and an error field for each, could we maybe give the option to pass in callbacks to the hook.

In my mind I'm thinking from an app developer point of view here. It might be slightly cleaner if we could pass in a callback that could handle local state. What do you think?

Eunovo commented 4 years ago

Hmmm... @kingsleyzissou It will certainly make things cleaner but let's imagine this scenario where a dev uses the results in components like this

<ShowContentOrLoader isLoading={isLoading}>
content
</ShowContentOrLoader>

How would the dev handle this using a callback?

kingsleyzissou commented 4 years ago

So I was thinking something like this:

const [loading, setLoading] = useState(true);

const { ... query stuff here ... } = useQuery(...options, setLoading) // or whatever callback

// in the component

return (
<ShowContentOrLoader isLoading={loading}>
    content
</ShowContentOrLoader>
)

This way, the value from the hook is just passed to the callback (which in this case is setState) and then the hook will update the local state for us.

kingsleyzissou commented 4 years ago

But your point is very valid. You probably wouldn't want the same loading component for mutations and queries... so either you could share the same local state or have separate local state for the two different hooks. This doesn't really change much, but might give developers more options

wtrocki commented 4 years ago

I think we should go with the first approach:

We need both. Lazy query is needed when you have conditionals etc. We can start with first one though. I think apollo has this concept of lazy query it has called flag returned by hook called boolean A boolean indicating if the query function has been called, used by useLazyQuery (not set for useQuery / Query). https://www.apollographql.com/docs/react/data/queries/

wtrocki commented 4 years ago

So.. I think we need:

For phase 1 let's forget about subscriptions, delta and lazy queries. We want plain hooks only. This will allow me to play with hooks on top of the apps we have written and see how they fit to many scenarios we seen.

@Eunovo Makes sense?

Eunovo commented 4 years ago

@wtrocki I will start plain hooks

kingsleyzissou commented 4 years ago

687