fix(types): switch order of schema type arguments - @macinjoke

[macinjoke]

Description

fixed this.

https://github.com/prescottprue/react-redux-firebase/pull/826/files#r386035738

Check List

If not relevant to pull request, check off as complete

• All tests passing
• Docs updated with any changes or examples if applicable
• Added tests to ensure new feature(s) work properly

Relevant Issues

[codecov]

Codecov Report

Merging #875 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #875   +/-   ##
=======================================
  Coverage   88.77%   88.77%           
=======================================
  Files          29       29           
  Lines         784      784           
=======================================
  Hits          696      696           
  Misses         88       88           

[prescottprue]

Switching these arguments was in attempt to get the schema feature added in this PR to work as expected. Is there a reason for going back? I understand that it became breaking for this switch, but seems like the right move since it was the original intention. What are your thoughts?

[macinjoke]

Sorry, it was an inadequate change.
I should have also switched the parts that use FirebaseReducer.Reducer. So I changed firebaseReducer and makeFirebaseReducer as well. 5929bcb

The reason for ordering the FirebaseReducer.Reducer type arguments in the order of Scheme, ProfileType is simply that TypeScript throws an error, as shown in the screenshot image. TypeScript generics cannot bring ordinary arguments after arguments with default values. (Because the first default argument doesn't make sense)

[prescottprue]

It seems like the the ProfileType is set to an object already, so we should be able to remove the default setting there right?

[macinjoke]

It seems like the the ProfileType is set to an object already

It seems that the ProfileType set here has nothing to do with the ProfileType of FirebaseReducer.Reducer.
The type argument ProfileType used in generics can be either Foo or Bar.

I think the type argument ProfileType is necessary for the user to specify a type like firebaseReducer<Foo, Bar>. (In this case, Bar becomes the ProfileType.)

[prescottprue]

Thanks for the PR. Released in v3.2.0

[rscotten]

@prescottprue, @macinjoke

Guys! Stop changing the order! This is going to break our code again!

@prescottprue I wish you would have asked me about this.

Just give both types defaults like this:

export function firebaseReducer<
  UserType,
  Schema extends Record<string, Record<string | number, string | number>>
>(state: any, action: any): FirebaseReducer.Reducer<UserType, Schema>

export function makeFirebaseReducer<
  UserType = {},
  Schema extends Record<string, Record<string | number, string | number>> = {}
>(): (state: any, action: any) => FirebaseReducer.Reducer<UserType, Schema>

export namespace FirebaseReducer {
  export interface Reducer<
    ProfileType = {},
    Schema extends Record<string, Record<string | number, string | number>> = {}
  > {

This way nobody needs to add the UserType or Schema if they don't want to.

And UserType should always come first because it's more important than Schema.

Please make the changes I gave above or let me know and I'll submit a PR to make the change.

[rscotten]

@prescottprue @macinjoke I submitted PR #888 with the preferred approach to solving this type error.

[rscotten]

@macinjoke You could have solved this problem without creating a PR with breaking changes simply by doing this:

  FirebaseReducer.Reducer<{}, {}>

[prescottprue]

Thanks for the PR @rscotten! For some reason I thought that is what the last PR is doing, which is why I included it in 3.2.0.

I'll get this out right away

[rscotten]

@prescottprue Thank you! :)

[macinjoke]

This way nobody needs to add the UserType or Schema if they don't want to.

And UserType should always come first because it's more important than Schema.

Please make the changes I gave above or let me know and I'll submit a PR to make the change.

@prescottprue @macinjoke I submitted PR #888 with the preferred approach to solving this type error.

@macinjoke You could have solved this problem without creating a PR with breaking changes simply by doing this:

  FirebaseReducer.Reducer<{}, {}>

Sounds great! Thank you.

[JonnyBoy333]

Would someone here be kind enough to share an example of how to use the Schema part of FirebaseReducer.Reducer(ProfileType, Schema)? Seems like no matter what I put in there I get the error:
Type 'DBSchema' does not satisfy the constraint 'Record<string, Record<string | number, string | number>>'. Index signature is missing in type 'DBSchema'.

Trying to do something simple like:

interface IContact {
  name: string;
  email: string;
}

// create schema for the DB
interface DBSchema {
  contacts: IContact
}

export interface RootState {
  firebase: FirebaseReducer.Reducer<IProfile, DBSchema>; // <--- Error here
  firestore: FirestoreReducer.Reducer;
  localSettings: ILocalSettings
  loading: ILoading
}

const appReducer = combineReducers<RootState>({
  localSettings: settingsReducer,
  loading: loadingReducer,
  firebase: firebaseReducer,
  firestore: firestoreReducer,
});

[rscotten]

@JonnyBoy333 The original PR was #826 which broke the codebase due to putting the new Schema type before the Profile type in FirebaseReducer.Reducer, which was later fixed. So make sure you're on the latest version (3.3.0).

This is actually a known issue. The author of this change references it here:

#826 (comment)

[rscotten]

@JonnyBoy333 you can hack it by doing either:

combineReducers<RootState>({
    // @ts-ignore <-- you can add this
    firebase: firebaseReducer as any, // <-- or this
  });

[rscotten]

@JonnyBoy333 figured it out. The type definitions need to be changed in index.d.ts:

export function firebaseReducer<
  UserType extends Record<string, any> = {},
  Schema extends Record<string, any> = {}
>(state: any, action: any): FirebaseReducer.Reducer<UserType, Schema>

export function makeFirebaseReducer<
  UserType extends Record<string, any> = {},
  Schema extends Record<string, any> = {}
>(): (state: any, action: any) => FirebaseReducer.Reducer<UserType, Schema>

[rscotten]

@JonnyBoy333 @prescottprue submitted PR #906 to fix this.

[JonnyBoy333]

@rscotten Wow that was quick, thank you and also thanks for updating the documentation.

I tried copying your updated index.d.ts into my project and I am still getting an error (even with your documentation example). It's the same error as before and I think it's because there is another reducer type on line 1134. Here:

react-redux-firebase/index.d.ts

Line 1134 in cfc516c

Schema extends Record<string, Record<string | number, string | number>> = {}

This is what gets referenced when you call FirebaseReducer.Reducer

[rscotten]

@JonnyBoy333 Good catch. Fixed. Thanks!

[JonnyBoy333]

@rscotten Confirmed, no more errors here.

I hope someday someone will do the same for Firestore schemas 🤞.