[Feature] persist results from find, get service methods in store

petermikitsh commented 7 years ago

It would be useful if the results of service get or find methods were also stored in store.records. Currently, they are cached in the queryResult key.

In my app, when server side rendering, I populate store.records with the relevant results for a given route (say the homepage - /).

If the user doesn't begin their browsing on that page, the server-provided store won't have records. When the user eventually navigates to /, I want to initiate the call to get the records in my componentDidMount hook in React. But then the results aren't store.records.

I could merge the data from queryResults and store.records (throwing away any duplicates). At best, that's a hacky fix because the next query I perform will clear the previous data in queryResults -- and suppose the next query is a subset of the current results, then I'm throwing away data.

It would be preferable to have a single source of truth for all data for any particular service. Any time I call get or find, the data is persisted in store.records. It's different from the current behavior -- store.records stores records received from realtime events only.

eddyystop commented 7 years ago

"I want to initiate the call to get the records in my componentDidMount hook in React. But then the results aren't store.records."

Why not call the repo's store method in the .find()s .then() ?

petermikitsh commented 7 years ago

@eddyystop You mean like this? (find and store are the redux action creator functions).

  componentDidMount() {
    this.props.find({query: {...}})
      .then(({ value }) => (
        this.props.store({records: value})
      ));
  }

That overwrites the previous array of store.records. It doesn't merge them.

eddyystop commented 7 years ago

I've read your issue several times and I wrote a long reply but the short of it is this. store.records is intended for replication engines which replicate part of a file. Only they know the selection criteria, so Feathers calls cannot be merged into store.records.

You seem to be using store.records as a cache. You are not taking mutations by others into account because I think you are using it only server side.

The approach I suggest is to implement a cache with a reducer which acts on the repo's _FULFILLED action code for the service. It can either use store.records or extend the repo's store. You could use the STORE action code to clear store.records.

petermikitsh commented 7 years ago

Hmm, I'll explore that more. Thanks for the pointers. With regards to this project, would you consider caching functionality in / out of scope?

subodhpareek18 commented 7 years ago

I had similar queries earlier, was confused about data queryResults and store all being present at the same time. But after I put feathers-offline-realtime into the mix, things made a lot more sense, so you should very much try them out.

It might be seem a little bit convoluted at first to use so many modules together, but it does make sense.

Finally about the caching functionality, I don't think it should be done with this library. The idea here is to only provide bindings into the redux store and make it very easy to work with it.

It's better to always persist the state of a redux store at the redux level, that ways you can be very precise about what state you want to persist, etc. It could be something outside of feathers, plain UI state, or it could be just the auth part of the feathers services, etc etc

Some links: https://egghead.io/lessons/javascript-redux-persisting-the-state-to-the-local-storage https://github.com/rt2zz/redux-persist

eddyystop commented 7 years ago

I was thinking that Peter's need might possibly be recast into something generally useful.

The repo does not effectively support paginated .find()s. That could be changed so that a paginated find adds to end of queryResults, though it overwrites records with the same _id || id. (This might have to be an option sigh to remain backward compatible.)

There needs to be a way to clear queryResults even if its something as silly as doing a .find() with query: { $limit: 0 } }.

I assume Peter can do .find() instead of .get() to handle his issue. And in addition the repo can support paging scrolls, which strikes me as being similar to what Peter's doing.

Does that work, Peter? Subodh, Peter, any suggestions on how to clear queryResults?

petermikitsh commented 7 years ago

Hi Eddy,

I've been thinking about this more -- and I think you are right, that I'm looking for a cache. I believe these to be my requirements:

Must be able to populate records when performing server side rendering. That means serializing the store, and injecting it at the top of the HTML document, and storing it in a value like window.__INITIAL_STATE__. The store is then rehydrated when the client JS is loaded. Currently I am doing this with no issue. (This process is necessary so React's checksum validation succeeds).
Wherever the records are stored, when new events arrive (e.g. created, updated), the records are appropriately updated.
Whenever a find or get occurs, and there are new records found, they are added to the store.

It is possible that I'm not approaching this the best way, and not leveraging the library's existing functionality. As I see it, the best solution would be the addition of a new key (cache) within the service's store that meets the requirements above.

In terms of what this means for the implementation, the fulfilled reducer and the store reducer would be responsible for adding to, or updating, records in the cache.

I'm not currently using pagination, but it's likely I will one day.

When supplying redux state to my react component props, I would point to the cache. If I wanted to further filter down or sort the records, I could use the standard filter or sort methods.

petermikitsh commented 7 years ago

The only thing that would stop me from using an augmented implementation of queryResults (included your suggested changes) would be that the records held in queryResults are not updated in realtime.

eddyystop commented 7 years ago

"Wherever the records are stored, when new events arrive (e.g. created, updated), the records are appropriately updated. Whenever a find or get occurs, and there are new records found, they are added to the store."

You will have to take reconnects into account, as your cache may be out of date on a reconnect. Disconnects will occur on mobile more than on browsers.

You will also have to send all service events to the client unless the server knows which records each client has cached. This is not easy to do. You can see feathers-offline-publication for the only way I could find to do something similar.

petermikitsh commented 7 years ago

I hadn't considered offline use cases.

You will have to take reconnects into account, as your cache may be out of date on a reconnect. Disconnects will occur on mobile more than on browsers.

Possible solution: re-execute all relevant queries once reconnected. It seems that this would be out of the scope of this library to know which previously ran queries are currently relevant to the application state. If you stored the connection state in redux, it'd be easy to watch when it changed, and then re-query -> and update the cache.

In my use case, it might be as simple as forcing React to re-render on reconnect to ensure all componentDidMount lifecycle hooks run -- that's where I initiate my queries.

You will also have to send all service events to the client unless the server knows which records each client has cached.

Yeah, I definitely don't want to get into the business of the server knowing what the client has cached.

Doesn't feathers send all service events to all clients by default? I was working under this assumption.

Before I was using this library (but I was using feathers-client and redux), I was capturing service events and dispatching accordingly (lightly edited, but conceptually complete):

const feathersClient = feathers(). ...;
const services = [...];
const store = configureStore(window.__INITIAL_STATE__);

services.forEach(function (serviceName) {
  const service = feathersClient.service(serviceName);

  ['created', 'updated', 'patched'].forEach(function (serviceAction) {
    service.on(serviceAction, function (record) {
        store.dispatch({
          type: 'SAVE_RESOURCE',
          value: {
            serviceName,
            resource: record
          }
        });
      }
    });
  });

  service.on('removed', function (record) {
    store.dispatch({
      type: 'DESTROY_RESOURCE',
      value: {
        serviceName,
        id: record.id
      }
    });
  });
});

eddyystop commented 7 years ago

Feathers does send service events to all clients by default.

On Sat, Aug 12, 2017 at 2:15 PM, Peter Mikitsh notifications@github.com wrote:

I hadn't considered offline use cases.

You will have to take reconnects into account, as your cache may be out of date on a reconnect. Disconnects will occur on mobile more than on browsers.

Possible solution: re-execute all relevant queries once reconnected. It seems that this would be out of the scope of this library to know which previously ran queries are currently relevant to the application state. If you stored the connection state in redux, it'd be easy to watch when it changed, and then re-query -> and update the cache.

In my use case, it might be as simple as forcing React to re-render on reconnect to ensure all componentDidMount lifecycle hooks run -- that's where I initiate my queries.

You will also have to send all service events to the client unless the server knows which records each client has cached.

Yeah, I definitely don't want to get into the business of the server knowing what the client has cached.

Doesn't feathers send all service events to all clients by default https://docs.feathersjs.com/api/events.html#event-filtering? I was working under this assumption.

Before I was using this library (but I was using feathers-client and redux), I was capturing service events and dispatching accordingly (lightly edited, but conceptually complete):

const feathersClient = feathers(). ...;const services = [...];const store = configureStore(window.__INITIAL_STATE__); services.forEach(function (serviceName) { const service = feathersClient.service(serviceName);

['created', 'updated', 'patched'].forEach(function (serviceAction) { service.on(serviceAction, function (record) { store.dispatch({ type: 'SAVE_RESOURCE', value: { serviceName, resource: record } }); } }); });

service.on('removed', function (record) { store.dispatch({ type: 'DESTROY_RESOURCE', value: { serviceName, id: record.id } }); }); });

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/feathersjs/feathers-redux/issues/24#issuecomment-321997300, or mute the thread https://github.com/notifications/unsubscribe-auth/ABeznwgMkv-0Cda84lN8W2W66oOplmgeks5sXevOgaJpZM4O0Vdv .

eddyystop commented 7 years ago

I think we can close this. Feel free to reopen it.

feathersjs-ecosystem / feathers-redux

[Feature] persist results from find, get service methods in store #24