feat(types): add support for typed db schema

[vicrac]

Description

For now, loose typings for state make it unsafe to use with typescript. I've added additional type parameter to state, defining Schema for database. Consider:

interface Todo {
  description: string;
}

// create schema for the DB
interface DBSchema {
  todos: Todo;
}

const rootReducer = combineReducers({
  firebase: makeFirebaseReducer<DBSchema>()  // new API for passing type parameter
})

type AppState = ReturnType<typeof rootReducer>;

Now if we create a selector...

export const firebaseSelector = (state: AppState) => state.firebase;

export const allItemsSelector = createSelector(
  firebaseSelector,
  firebase => firebase.data.todos
);

we are getting autocompletion!:

... and proper inner type!

Of course, I am open to suggestions, but I literally made the smallest possible change that allowed me to get these features (and not to crash types).

[codecov]

Codecov Report

Merging #826 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #826   +/-   ##
=======================================
  Coverage   91.43%   91.43%           
=======================================
  Files          29       29           
  Lines         759      759           
=======================================
  Hits          694      694           
  Misses         65       65

[prescottprue]

Amazing work! Thanks for the PR! I think this is part of what what was being described in #670

Since this doesn't seem to be breaking in anyway we should be able to get this out in a new minor version

[vicrac]

That's great 😄 I gave it a bit thought and here are things that we should consider (but note it's not necesarrily to be implemented now. I think these types should be rather released as soon as possible, feedback should be gathered and then they should be gradually improved):

1. Doing the same kind of types for firestore
2. I realized currently my solution works only for not-nested objects, due to Record<string | number, string | number> given as type. There should be developed a generic type for any kind of data that can be stored in database (it will probably be a recursive type, sth like type DBObject = Record<string | number, string | number | DBObject>) if there's no such yet.
3. What if schema for DB contains not only collections, but also some "unique" object (e.g. for storing denormalized counters)? I mean

{
  todos: { ... },
  counters: {
    todos: 15
  }
}

it should be reflected in schema so it doesn't produce a Record from it. I thought of sth like:

import { Collection } from 'react-redux-firebase';

interface Counters {
  todos: number;
}

interface Schema {
  todos: Collection<Todo>;
  counters: Counters;
}

and then branching on it using most probably conditional types.
4. I've also seen people doing some nesting for joins' purposes, e.g

{
  users: { 
    1: { name: "John Doe" }
  },
  todos: {
    1: {  // user's 1 todos
      2: {
        description: "Some todo"
      }
    }
  }
}

(then retrieving todos for user 1 is simpler). How should it be handled? How is it currently handled by implementation itself?

[vicrac]

Guys, any conclusions on above? I'd love to have this feature in my project :)
@prescottprue

[prescottprue]

Love the idea you are mentioning above, but agree it can come at a later time.

Going to point this to v3.1.0 and so that we can get it released - I'm thinking we will also want to add something about this to the documentation for other folks to get the same benefits.

Is the makeFirebaseReducer function necessary or have you found a way for the type to be passed with the default reducer import and assignment? Either way, it would also be nice to include an explanation of how this is working as part of the documentation

[vicrac]

@prescottprue Great. Unfortunately, makeFirebaseReducer is necessary to pass type parameter - Typescript doesn't have polymorphic literals, so you can't do

combineReducers({
  firebase: firebaseReducer<DBSchema>  // <-- won't compile
})

Also, I don't really have free time to polish it nor write proper documentation, but in case of any questions regarding Typescript/anything else I'd be happy to help 👍

[rscotten]

@prescottprue @vicrac
I love the idea and intention here, but this PR is incomplete, and its release a little premature because it broke my typed code.

1. It only defines a Schema for Firebase Realtime DB, not Firestore.
2. It really needs some documentation on how to implement. Like, how do I type this?

import { combineReducers } from 'redux';
import { firestoreReducer } from 'redux-firestore';
import { firebaseReducer } from 'react-redux-firebase';

interface User { };

interface Schema { };

interface RootState {
  // previously
  firebase: FirebaseReducer.Reducer<User>;
  // change to
  firebase: FirebaseReducer.Reducer<Schema, User>;
  firestore: FirestoreReducer.Reducer;
}

combineReducers<RootState>({
    firebase: firebaseReducer,
    firestore: firestoreReducer,
  });

I admit this change is not too difficult for a Typescript developer to figure out, but documentation with this release would have saved me some debugging time.

[rscotten]

Also, Schema should have been made the second generic parameter to FirebaseReducer.Reducer instead of replacing UserType as the first. This is going to cause everyone's code to break.

[prescottprue]

@rscotten Thanks for pointing out - I'm open to a PR if you get a chance

[w0nche0l]

Yeah this was definitely an annoying breaking change to figure out, please put it in the patch notes or something?

[prescottprue]

@w0nche0l The support for schema was mentioned in the release notes, but the break of types was not intended as part of the change. Instead of noting the workaround I am making the update mentioned by @rscotten (i.e. having Schema be the second generic type). Also, I have been working on updating the typescript example and adding a page to the recipes section of the docs explaining how to pass a schema type when using Typescript.

As for the schema handling for Firestore, I'll have to get to that when I can unless someone else wants to make a PR

[macinjoke]

This is breaking change. The parameter order was changed.

[prescottprue]

Good call out. Yeah, this was in an attempt to have the feature work as originally intended