Add addDoc function overloading to accept DocumentRef

[ikenox]

The current addDoc interface does not provide a way to specify the document ID to add, so I added addDoc function overloading to also accept DocumentRef.

[changeset-bot]

🦋 Changeset detected

Latest commit: e2d8f93

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	Patch
@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

[tom-andersen]

Thank you for your contribution.

I think the functionality you want can be found in setDoc. Is there something semantically different that I missed?

[ikenox]

Hi @tom-andersen, thank you for the review!

The difference is:

• setDoc overwrites the existing doc, so-called "upsert" behavior.
• addDoc is just an "insert". It throws an error if the document already exists (the test case I added).
  • This behavior is useful to ensure that the doc is newly created and existing one is not overwritten.

@google-cloud/firestore actually provides the both way by DocumentRererence.set and DocumentReference.create.

[tom-andersen]

@ikenox Thanks for the quick response.

Having addDoc fail is a problem when client is offline, and the write was cached. Operations that can fail due to conflicting data are restricted to transactions which only run while online.

Introducing writes that can fail due to conflicts while offline is something I am against. Or atleast require better error handling than currently present.

Likewise, transactions do not have the addDoc variant that generates a document id, since transactions are supposed to be idempotent. A function that generates a different document id on every retry is a problem. This is due to web/mobile clients using lockless optimistic concurrency to better support unreliable networks.

We could consider adding the addDoc variant that takes document reference to the transaction, as that will restrict operation to online only and be idempotent.

I have brought this up for team discussion, but without a compelling argument for adding this method and how to better handle errors while offline, we cannot move forward.

The best workaround I can offer at this time is to use a transaction, get for existence, and then set if it does not.

Your input is appreciated.

[ikenox]

@tom-andersen

With consideration of offline scenarios, I noticed that the thing is more complex than my initial thought.

The best workaround I can offer at this time is to use a transaction, get for existence, and then set if it does not.

I think this is sufficient workaround.
So I'll close this PR. Thanks for the discussion!