types(firestore): make withConverter retro compatible

[posva]

I found this while migrating vuefire to Firebase 10: it was no longer possible to write:

itemRef.withConverter<number>({
  // ...
})

As it would fail in TypeScript. I think adding the default type value here helps people migrate to Firebase 10.
I haven't added (or even checked where to add) a test because I'm unaware of any discussion that happened for this.

It's also possible that other places would benefit from a similar change of adding a default TS parameter

[changeset-bot]

🦋 Changeset detected

Latest commit: 31b6e42

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@firebase/firestore	Minor
firebase	Minor
@firebase/firestore-compat	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[ehsannas]

Thanks for your contribution @posva .

@dconeybe PTAL

[dconeybe]

Thank you for the contribution! Could you show a complete code sample that used to compile in v9 and doesn't compile in v10? I'd like to copy/paste it and see what other places could benefit.

[posva]

Sure:

import { collection, getFirestore, type DocumentData } from 'firebase/firestore'

const db = getFirestore()
collection(db, 'todos').withConverter<
  { text: string; finished: boolean },
  DocumentData
>({
  fromFirestore: (snapshot) => {
    const data = snapshot.data()
    // ... do actual checks
    return { text: data.text, finished: data.finished }
  },
  toFirestore: (todo) => todo,
})

The 2nd generic is now required but it wasn't before. itemRef could also be a doc() (from the original post)

[posva]

Hello! any news @dconeybe ? The later this is merged the less value is has as the point is to ease the upgrade from firebase 9 to 10 for users

[dconeybe]

Thank you for your contribution, and apologies for the delay in providing feedback. After discussing with the team, we have decided to incorporate your suggestion. Since this change involves a public API change there is some additional process that we need to go through to get it approved; however, I do not expect any issues. But it may be 1-3 weeks to get approval to merge it. Also note that we have incorporated this suggestion into our node server sdk (googleapis/nodejs-firestore#1887), which has less a less stringent process for public api changes. In the interim, I've left a bit of feedback. Please take a look. Thanks again for contributing to Firestore!

[dconeybe]

I'm looking into the GitHub Actions failures of "Deploy Project Config" and "Update API reports". It appears that they may need to be adjusted to support external contributions.

Denver implemented the changes.

[tom-andersen]

@markarndt I think we need your approval on changeset before we can merge. Could you take a look?

[tom-andersen]

@hsubox76 I think I need your approval.