withConverter: multiple data types

[Ehesp]

Hello,

The withConverter API currently accepts a single type, for example:

class User { ... };

const converter: FirestoreDataConverter<User> = {
  toFirestore(user: User): DocumentData {
    return {
      full_name: user.first_name + ' ' + user.last_name,
    };
  },
  fromFirestore(snapshot: QueryDocumentSnapshot): User {
    return new User(...);
  }
}


const users = firebase.firestore().collection('users').withConverter<User>(converter);

// Usage when adding to Firestore
await users.add(new User(...));

Whilst using Firestore on a single platform (e.g. web) this is fine since I'm able to use the returned User instance in my code. This pattern however does not work when I need to deserialise the data. For example, an API endpoint needs to return a JSON serialisable response, and in this case the class User needs to be transformed, e.g:

app.get('/user', (req, res) => {
  const snapshot = await users.doc('...').get();

  // User class
  const user: User = snapshot.data();

  // Custom .toJson() method on the User class
  return res.json(user.toJson());
});

Although this is ok, it does lead to code duplication and potentially forgetting to call toJson(). Instead, would it be possible to provide a different fromFirestore data type in the withConverter API? Something like:

  export interface FirestoreDataConverter<T, R = T> {

    toFirestore(modelObject: T): DocumentData;
    toFirestore(modelObject: Partial<T>, options: SetOptions): DocumentData;

 
    fromFirestore(snapshot: QueryDocumentSnapshot, options: SnapshotOptions): R;
  }

Instead, I could declare the converter as so:

interface IUser {
  full_name: string;
}

class User { ... };

const converter: FirestoreDataConverter<User, IUser> = {
  toFirestore(user: User): DocumentData {
    return {
      full_name: user.first_name + ' ' + user.last_name,
    };
  },
  fromFirestore(snapshot: QueryDocumentSnapshot): IUser {
    return {
      full_name: snapshot.data().full_name,
    };
  }
}


const users = firebase.firestore().collection('users').withConverter<User, IUser>(converter);

How the return type defaults to the original type (keeping backwards compatibility) but also allows to return a different type:

app.get('/user', (req, res) => {
  const snapshot: DocumentSnapshot<IUser> = await users.doc('...').get();

  // Returns as serialisable data
  return res.json(snapshot.data());
});

[Feiyang1]

@schmidt-sebastian

[thebrianchen]

@Ehesp Thanks for this well thought out request! This is definitely something we can add as it is a non-breaking change. We're currently working on getting other QoL improvements shipped, but I'll revisit this in the future.

[Ehesp]

Having worked with converters on a few different projects now, I'm actually finding this to be a type limitation which is frequently popping up. Take the Firebase Toxicity Extension for example, it modifies documents with data based on what you enter (e.g. a Map called attribute_scores).

When adding a document in the current state, my code looks like so:

type Review = {
  attribute_scores: {
    TOXICITY: number;
  };
  message: string;
}

.withConverter<Review>(...)

The issue is, when calling say setDoc, it's expecting me to provide an attribute_scores, even though this is provided by the extension. On my reviews listing, I filter only where the review has a toxicity rating lower than X (hence why it's a required attribute). Ideally, this would work better as:

type Review = {
  message: string;
}

type ReviewDocument = {
  attribute_scores: {
    TOXICITY: number;
  };
} & Review;

.withConverter<Review, ReviewDocument>(...)

[thebrianchen]

@Ehesp That's a good use case! We currently are prioritizing other features at the moment, but we'll definitely come back to this in the future.

[tzappia]

Seems like this has been implemented in the modular js sdk, but not yet in the admin nodejs sdk. It's a shame there wasn't an easier way to keep the js and admin capabilities in sync :(

[lucastnr]

I've spent hours trying to make this work but it seems like a type limitation in the SDK itself. I can't find a way to fix this without patching the library myself.