feat(firestore): `firestore.status` is updated when using watchEvent

[Venryx]

Do you want to request a feature or report a bug?

Bug.

What is the current behavior?

When using firestore, the "firestore.status" path is always empty in its "requesting", etc. children.

What is the expected behavior?

For the "firestore.status" subtree to correctly hold the statuses of requested paths.

The problem seems to be that redux-firestore's statusReducer waits for the actionTypes.START and actionTypes.SET actions. However, these are never dispatched when using the firestore backend.

The only place which dispatches a START action for example, is this line in query.js of react-redux-firebase:

export const watchEvent = (firebase, dispatch, options) => {
    [...]
    dispatch({ type: actionTypes.START, path: storeAs || path })
    [...]
}

And the only code which calls the watchEvent(s) function is in the firebaseConnect.js file.

Thus, when using firestore, the START action is never dispatched, so firestore's statusReducer never changes its data, so the firestore.status path in the store is always empty.

[prescottprue]

You are correct about it being due to redux-firestore within the statusReducer.

One thing to note as well is that setListener is the new method for Firestore (which comes from redux-firestore) instead of watchEvent. There is also setListeners for an array.

If things aren't working as expected with that, it is most likely the place were we will want to look to dispatch a similar event type (but from redux-firestore). It may be good to support usage of watchEvent to make Firestore queries as well, but it seems like it may cause confusion.

[prescottprue]

@Venryx Unless you are hoping to also use watchEvent with Firestore (which is not currently a feature as mentioned) not sure that this will make much sense. Going to close for now, reach out if you want to reopen.

[Venryx]

Well, at the moment it's kind of a bug since it creates a node in the redux-store at "firestore/status/[requesting/requested]", but doesn't populate it. So if the "status" path is not meant to be used in firestore, the firestore reducer should not even be creating that node.

That said, it seems like "firestore/status/[requesting/requested]" would be useful in the same way that "firebase/[requesting/requested]" is -- as an easy way to determine if a path has finished loading/retrieval. (indeed, that's what I used it for in my previous Firebase project, and was hoping to use it in my new Firestore one as well)

If you're just short on development time for it, though, that's understandable. Maybe I or someone else can add a Firestore equivalent of "actionTypes.START" and "actionTypes.SET", and have the Firestore "status" reducer updated to check for them.

[prescottprue]

@Venryx That piece of state is supposed to be used with Firestore, but that is not updated by watchEvent (since to call data from firestore you use setListener). That said, support for setListeners updating that state is under way.

In the PR for v0.3.0 there are changes which update state.firestore.status when queries are created through setListener (what firestoreConnect uses internally). I believe this is what you are looking for, but just wanted to clarify that that DOES NOT change state.firestore.status when watchEvent is called (only setListener or `setListeners).

[Venryx]

Okay, sounds good then; I use setListener in my customized firestoreConnect function, so if state.firestore.status gets updated from there, then no need for a further pull-request.

[prescottprue]

If you have time I would be interested to see what you have customized in your own firestoreConnect so we can see if it fits in anywhere (we can chat on it over gitter).

If it has to do with only attaching one listener for a certain path you may be in luck - v0.3.0 of redux-firestore also adds automatic de-deduping of listener callbacks (with config to re-enable this if it is intended).

[Venryx]

Sure. Basically, my custom version merges FirebaseConnect with Redux's standard Connect.

So instead of writing:

@firebaseConnect(()=> [
    "path/to/data",
])
@connect((state, props)=> ({
    data: state.firebase.data.path.to.data,
}))

You would just write:

@Connect((state, props)=> ({
    data: GetDatabaseData("path/to/data"),
}))

The customized Connect function tracks the paths requested through the GetDatabaseData calls, and then auto-requests those from Firebase. Then, when that data gets loaded in, the connector function is called again, triggering the "next step" of processing and data-requesting. This process can repeat a number of times as it gets incrementally more of the data it needs.

I like having the two decorators merged, for two main reasons: 1) It's simpler/less redundant. You ask for the data once, and it does both steps. (getting from database, and adding as passed-prop for component) 2) It allows you to have complex data retrieval trees, that are kept encapsulated in a single function. This way, those data retrieval trees can just request whatever paths they want, without having the developer remember to add a corresponding entry in the firestoreConnect decorator.

I could explain more, and give examples of how it's been helpful for me, but I am a bit lazy right now. If people need some "real examples", you can look through the data-retrieval functions present under this folder of one of my projects: https://github.com/Venryx/DebateMap/tree/c12ea6c7fe78919d9f9d2bf6f22af90d272c011a/Source/Store/firebase

Anyway, now that we've gotten the "what it does" out of the way, here is the actual source code for it: https://github.com/Venryx/DebateMap/blob/c12ea6c7fe78919d9f9d2bf6f22af90d272c011a/Source/Frame/Database/FirebaseConnect.ts

I will be the first to admit that it is messy, and quite devoid of comments. I apologize. So far, I am the only developer on the project, so I have not put too much effort on documenting things or refactoring them so far.

The basic idea of the customization is pretty simple, but my implementation is kind of a hack at the moment. It has been working however, and has given me a lot of extra flexibility in my data-retrieval functions. (the "selector" functions called from the merged firebaseConnect/connect functions)

[Venryx]

By the way, in the data-retrieval functions referenced in the first link above, you'll notice a lot of calls to CachedTransform(...).

So what does it do? Basically, it is an alternative to "memoization".

I really didn't like how memoization broke the normal "encapsulation" that function trees provided. Instead, you would have to pay more attention to the dependencies between memoization functions; it led to specific problems that were very hard for me to work around in my data-retrieval trees. (unfortunately, I forget the details now -- I think it had something to do with "input-selectors" of memoization getting complicated when you wanted to add parameters to them)

Anyway, I was getting very annoyed with the memoization constraints. So I tried making a different system -- which is more flexible, and matches roughly how "shouldComponentUpdate" works for React components. Except instead of "shouldComponentUpdate" it's more of "shouldProcessingReoccur", and instead of passing old and new props, you pass old and new parameters. Only when the parameters change, does the wrapped processing function get recalled -- then the result is cached and used until the parameters change again.

This means that the result data is exactly identical to the data returned last time (same as with memoization). Thus, a shallowCompare shows no change, so components that use that data don't need to rerender.

There might already be something like this (let me know if find one!), but a brief search didn't provide anything, so I coded an implementation myself.

Anyway, the code for it is here: https://github.com/Venryx/js-vextensions/blob/master/src/Utils/VCache.ts

Actually, there are two versions of it. The first (the one above) is a simple comparing of old and new parameters.

There is also this version: https://github.com/Venryx/DebateMap/blob/c12ea6c7fe78919d9f9d2bf6f22af90d272c011a/Source/Frame/Database/DatabaseHelpers.ts#L428

This version is different in that it also auto-includes "store accessed paths" (which includes database requested paths) as "parameters" for the wrapped function. Which is nice since then deep data-retrieval trees can fully encapsulate their database-calls -- no need for you to manually include the paths as parameters higher up.

Here is an example: (from here)

export function GetPathsToNodesChangedSinceX(mapID: number, time: number, includeAcknowledgement = true) {
    return CachedTransform_WithStore("GetPathsToNodesChangedSinceX", [mapID, time, includeAcknowledgement], {}, ()=> {
        let nodeIDs = GetNodeIDsChangedSinceX(mapID, time, includeAcknowledgement);
        let mapRootNodeID = GetRootNodeID(mapID);
        if (mapRootNodeID == null) return emptyArray;

        let result = [] as string[];
        for (let nodeID of nodeIDs) {
            let node = GetNode(nodeID);
            if (node == null) return emptyArray;
            let pathToRoot = GetShortestPathFromRootToNode(mapRootNodeID, node);
            if (pathToRoot == null) return emptyArray;
            result.push(pathToRoot);
        }
        return result;
    });
}

The code in the wrapped function only runs when one of the store-paths that it accessed last time, has changed since then. If so, the wrapped function is recalled, and it's output is stored in the cache for the given context.

The "context" is determined using the three "static parameters"/"context identifiers" -- mapID, time, and includeAcknowledgement in the example above. (a separate cache is used for each combination of values for those three variables)

I know that coding approaches like this are not preferred by everyone. Some people like when function trees are "fully encapsulated" so that you can just call it, and the database-retrievals, cache-enabled processing, etc. all happen behind the scenes. The functions above allow you to use this philosophy to a greater extent with React, Redux, and Firebase.

For others, they prefer these things to be made explicit, at a high/early level. For example, by specifying each database path that a component needs, directly within the component's firebaseConnect decorator. For people who prefer this more explicit approach, the functions above are likely to be seen as obscuring. For me, however, I consider them freeing; ie. more flexible/functional/encapsulating.

[prescottprue]

@Venryx If I am understanding correctly, we have felt a bit of this pain when scaling our react redux firebase applications as well. Have you tried using a combination of 'once' and 'child_changed'` for your list views? Then you are attaching listeners that will only update the single piece of the redux tree instead of the whole list.

Something else to checkout is reselect which allows you to make custom equality checking and memoization for cache capability.

I'll check out the memoization examples you posted and see if there is anything that seems like it could fit into react-redux-firebase, but I am open to suggestions.

[Venryx]

Yeah, reselect is actually the solution I was talking about which was less flexible.

It's not that it wasn't able to do memoization for my use-cases, it's that it was more complicated to do so because my data-retrieval functions went deep, and there were complications I kept hitting when trying to use it. From what I remember, it was because of the way reselect relied on hoisted-variables in function instances, rather than a "flat" data store for the cache, meaning the passing of parameters became increasingly complex as multiple data pieces were combined in the memoization functions. Most projects don't have this problem as much, because they don't rely on deep selector/data-retrieval-and-processing functions as much.

Anyway, the "store" for the cache data in my approach does have a higher overhead (having to join the parameters into a key which is used to check the cache), but it's still much faster than no cache at all -- and fast enough for my purposes, while being more convenient to use in my project's deep function trees.

I'm guessing that most people will prefer the standard memoization functions in reselect, since it has less overhead, and works fine for simpler cases. But I wanted to reference this alternate approach for anyone who's hit the problems I did and were looking for other options.