prescottprue / react-redux-firebase

Redux bindings for Firebase. Includes React Hooks and Higher Order Components.
https://react-redux-firebase.com
MIT License
2.55k stars 559 forks source link

Improve types for FirestoreReducer.Reducer #964

Closed msutkowski closed 4 years ago

msutkowski commented 4 years ago

Description

Expands on #957. Allows order and data to be correctly typed when there are nested collections/schemas. I can merge this into that branch once I get feedback on the below. Here is a TS playground showing the type resolution:

Example usage:

export interface FirestoreTask {
  target: string;
  from: string;
  direction: "inbound" | "outbound";
}

interface FirestoreSchema {
  organizations: {
    tasks: FirestoreReducer.Entity<FirestoreTask>;
  };
}

// rootReducer.ts
combineReducers({
    firestore: firestoreReducer as Reducer<
      FirestoreReducer.Reducer<FirestoreSchema>
    >,
   ... rest
  });
export type RootState = ReturnType<typeof rootReducer>;

// selector.ts
export const selectFirestoreTasks = (state: RootState, key: string) =>
  Object.values(state.firestore.data.organizations?.[key]?.tasks || {});

Questions: Should this work even be done on this repo? It seems like there is some duplication between this repo and redux-firestore.

Todo:

Check List

If not relevant to pull request, check off as complete

Relevant Issues

codecov[bot] commented 4 years ago

Codecov Report

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

@@           Coverage Diff           @@
##           master     #964   +/-   ##
=======================================
  Coverage   88.33%   88.33%           
=======================================
  Files          29       29           
  Lines         797      797           
=======================================
  Hits          704      704           
  Misses         93       93           
msutkowski commented 4 years ago

Adding a couple of notes here: The example above was using nested data, which obviously goes against the recommendation of the docs. The good news is this typing approach is still valid when flattened. So - we'd be covered in the event folks have bad schemas (like me!) or follow the docs.

export const selectFirestoreTasks = (state: RootState, subdomain: string) =>
  state.firestore.ordered.tasks || [];

interface FirestoreSchema {
  tasks: FirestoreReducer.Entity<FirestoreTask>;
}

It was also recommended to do something like this on firestoreReducer so that TS users can have it typed easily:

combineReducers({ firestore: firestoreReducer.forSchema<Schema>() })

My only other thought regarding the reducer is that if we're passing in a schema, we should also use it to initialize the firestore reducer state to get rid of the extra initializer like in the selectFirestoreTasks selector above.

msutkowski commented 4 years ago

Closing this out and will open against the typescript branch of redux-firestore if desired.