React-redux-firebase: bug(populates): populates clears all data at root

[Domiii]

The following query causes all other data currently stored at users (which is the root) to be deleted. As soon as the populates portion is resolved, all my (self-managed) users are gone from my client (however the users path is still in the firebaseConnect mix):

populates: [
  { child: 'uid', root: 'users', keyProp: 'uid' }
]

UPDATE

At some point, this SET action is dispatched with path = users:

dispatch({
  type: SET,
  path,
  data: result,
  timestamp: Date.now(),
  requesting: false,
  requested: true
})

From the looks of it, that is because promisesForPopulate actually creates a big object with all the user children which then overrides all previous data at root (= /users), using the above SET action.

Possible Fix (1)

Each fetched result object should be set with it's own SET action. So instead of:

SET('/users', {
  uid1: {...},
  uid2: {...}
});

you get:

SET('/users/uid1', {...});
SET('/users/uid2', {...});

Possible Fix (2)

Add an ADD or UPDATE action which adds to rather than overrides the given data at the given root path:

ADD('/users', {
  uid1: {...},
  uid2: {...}
});

[prescottprue]

Have you tried using storeAs for your other users query?

[Domiii]

Thanks! I'll try that as a work-around. (Luckily my current code allows easily aliasing local and remote paths)

When do you think populate will be fixed though?

Something like this (in actions/query.js) should just about do it, should it not?

forEach(results, (result, path) => {
  forEach(result, (child, key) => {
    dispatch({
      type: SET,
      path: `${path}/${key}`,
      data: child,
      timestamp: Date.now(),
      requesting: false,
      requested: true
    })
  })
})

[prescottprue]

@Domiii placing the populate results in a different place in redux would also require modifying populatedDataToJS. Data is placed into redux under its root.

[Domiii]

I tried to give it a swing, even though NPM bugs kept holding me back.

I finally managed to add my own fork of react-redux-firebase as a local git submodule which is a bit of a pain (see below).

Results

I tested my fix and as you already pointed out it's not enough. Can you give a quick run-down of other things I need to take care of with some more detail?

FYI: I changed the code in actions/query.js to:

forEach(results, (result, path) => {
  forEach(result, (child, key) => {
    dispatch({
      type: SET,
      path: `${path}/${key}`,
      data: child,
      timestamp: Date.now(),
      requesting: false,
      requested: true
    })
  })
})

PS: (For those who are interested) How to set this up as an NPM submodule?

NOTE: I know this does not technically belong into this thread. If you don't need/want it, I'll delete it.

WARNING 1: Git does not make it easy to remove or change submodules, gotta move through a few steps there, but you can google that easily.

WARNING 2: NPM does not handle local sub modules well. Here is how I got it to work:

1. Follow this article to setup your git submodule and add it to package.json
2. git submodule update --init --recursive
3. cd local_modules/react-redux-firebase
4. npm install
5. cd ../..
6. npm update

FYI: This NPM bug explains how NPM neglects to install dev-dependencies before building for local submodules. I even had to run npm install from the submodule directory entirely to get all packages installed. If I don't, it keeps complaining that the jwt-decode package is missing. Congrats: You now have a second node_modules folder in your submodule which is entirely useless, but that's how things go :/

[prescottprue]

You shouldn't necessarily need to make this a submodule. If you are developing locally you should be able to npm link.

It will take more of a code change than transforming that one line. v1.5.0-alpha should include this functionality.

[Domiii]

Good news! I finally came around to try it!

Bad news: storeAs can only be set once for the query, and it does not really seem to be integrated with how populates pulls in child data.

In fact, storeAs is not passed to promisesForPopulate at all, as you can see in this line:

promisesForPopulate(firebase, dataKey, data, populates)

Hence, path in the following forEach loop is still equal to root, for every entry in the populates array (I debugged and double checked):

forEach(results, (result, path) => {
  dispatch({
    type: SET,
    path,
    data: result,
    timestamp: Date.now(),
    requesting: false,
    requested: true
  })
})

[Domiii]

I have also found out why it is buggy in the first place:

• The SET reducer just overrides all data at path, when new data comes in.
• If I'm not mistaken, populated child data never gets deleted from the state, just letting it dangle in there for all eternity.

What should happen instead, is:

1. SET should merge data. Or a new MERGE action needs to be defined for the default case, where data should be merged in, rather than SET/ overriden.
2. UNSET_LISTENER should not remove any data when the corresponding path is a subpath of any still active path.
3. UNSET_LISTENER should only remove all subpaths in the the corresponding path, that are not still active.
4. UNSET_LISTENER should respect populates settings, to also remove all currently cached children related to the corresponding query in the same way (with respect to all active paths).
5. For (2), (3) and (4) to work, we simply have to maintain a set of all currently active paths (including populates child paths). On the plus side: If we unified things in the above manner, taking care of the populates child data would not be too troublesome, since the merge + delete logic is already taken care of, just a few extra paths to handle.

While this is quite a bit of work, this would actually allow a lot more than just fix this bug:

1. This way, when a @firebaseConnected component gets unmounted, all it's data is cleanly removed (including it's populated child data), and:
2. We can have @firebaseConnect paths be subsets of one another without subscribed data of still active paths suddenly vanishing whenever a component with the same, super or child path is unmounted.

Here is a quick example for why we need to invest this much effort in better bookkeeping, and why it is not just an issue with populates:

---

@firebaseConnect(['/a'])
class A extends PureComponent ...

@firebaseConnect(['/a/b'])
class B extends PureComponent ...

@firebaseConnect(['/a/b/c'])
class C extends PureComponent ...

I did not try this out, but if I'm not mistaken, in the current version, unmounting any of the three components will entirely mess up the other two components:

• If you unmount A, then B and C will be left entirely without data.
• If you unmount B, then A will be missing a piece of data it originally had and subscribed to, and C will be left entirely without data.
• If you unmount C, then A and B will each be missing a piece of data that they said they wanted, and might actually need!

Or maybe I'm wrong, and this is already (partially) handled somewhere?

---

Of course, all of this gets a lot more troublesome once you take all sorts of query flavors in consideration, especially limit and equalTo queries -- but that's just how annoying data maintenance is.

I went through all this pain a few years back, when I invented my own data model to keep an accurate cache of server-side data, before meteor, redux et al were born. In my case it would have been impossible to be respective of all possibly query, since I worked against mysql which is infinitely(*) more powerful than the firebase query language; but as it stands right now, taking care of all firebase queries and keeping the redux state consistent, is actually possible. Maybe not anymore, once firebase v4 is out haha!

On the plus side: I don't think, we have to make any changes to populatedDataToJS :)

[prescottprue]

@Domiii Great input. Going to look into this a bit more. It would be nice to come up with a way to enable/disable this through config so that it remains backwards compatible (that way it can be done for a v1.* version).

[prescottprue]

192 posed and is a duplicate of this issue (noting just to keep track)

@Domiii Would be interested to see what you think of v2.0.0 with respect to sharing data from different queries (like you were mentioning above). Also, the listeners are tracked in state, so keeping an eye on which components set which listeners is much easier.

[Domiii]

Seeing how my workarounds are running out of juice, I feel more and more compelled dedicating some time to this, even though I basically have negative time to work on this...

My plan would be to track paths and add some sort of set-management library to help define all tracked paths as a holistic set that we add to and manage through all the paths provided via @firebaseConnect. (Luckily, set complement and difference operations won't be necessary.)

Specifically, it appears that we want to generalize getEventsFromInput to help manage that set.

@prescottprue Did anything else happen in the meantime that would conflict with this plan, or can I go ahead on this (if I can find the time in the next two weeks)?

Also, at which branch should I start?

[prescottprue]

@Domiii This would be awesome.

With v2 listeners are tracked in state already, which could make this quite bit easier. Other than that, this shouldn't be affected by too much that has happened recently.

If you are trying to add it to v2, then go with the v2.0.0 branch, but if you are adding it to v1, go with the v1.5.0 branch.

[prescottprue]

@Domiii v2.0.0-beta.8 support for this (thanks for the help @marekolszewski) through the MERGE action.

[Domiii]

That is truly awesome! Looking forward to the final version actually not overriding data or removing data when subscribing to supersets and subsets of data across different containers! :D

(sorry, I did not end up finding/allocating the time myself :/)