samuelgozi/firebase-firestore-lite

Should `ref.get()` throw a 404 error when requesting a missing document?

Closed this issue · 7 comments

samuelgozi commented

Hi everyone.
Since this question doesn't really have a correct answer I wanted to ask you guys, the community what do to.

Right now, when making a request for a document that doesn't exist by using the reference.get() method, the method will throw a 404 error.

I was asked by a member of the community to re-evaluate if this should be the case, and I think he has a point.

Option A:
Returning a 404 error forces you to use try/catch clauses and provide some logic to when the document doesn't exist, and in the end of the day, it "forces" the developer to write better code.

Option B:
The alternative is to return null or undefined, and that way even if the document doesn't exist, it is up to the developer to decide whether to handle that or not.

Other methods:
When calling update or set on a non-existing document the default behavior is:

  • set if the document doesn't exist the one with the same id will be created, and the data will be set as requested.
  • update an error will be thrown in order to avoid creating a partial document, however, this can be overwritten.

Please let me know what is your opinion on the matter.

issue-label-bot commented

Issue-Label Bot is automatically applying the label question to this issue, with a confidence of 0.80. Please mark this comment with 👍 or 👎 to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

  • 👍6
dflorey commented

I'm lazy and would prefer B. It would probably be nice to have an option (strict mode or something) that will then turn on Option A

samuelgozi commented

@dflorey Thanks for sharing your opinion. As for now, it seems like that would be the direction we will take.

aidarkhanov commented

In my opinion, the client library should not have opinions on how a server responds to the request. It is good to document unexpected behavior (code errors and messages) and encourage the library users to take errors responsibly. Just my 2 cents

  • 👍1
DaniyelMe commented

As I see it, returning undefined is more intuitive, as if it was an object that is not yet initialized.

Just to complete the discussion, I'll add that Firebase SDK handles it this way:
If there is no document at the location referenced by docRef, the resulting document will be empty and calling exists on it will return false.
So when calling docRef.get() they add exists property to verify if it... exists.

docRef.get().then(doc => {doc.exists ? true : false; });

This two-step process is redundant in my mind, though we can see that the developers migrating from Firebase SDK might expects this behavior. Meaning -

  1. Not getting an error.
  2. A way to verify.

Thanks for the discussion, stay safe.

  • 👍3
sesam commented

Yes, the official API gives you an object { exists: true, data: () => { /* actually read & parser & return document object */ } So @samuelgozi have you seen if there's merit for delaying the document parsing and loading like the official API does with the data() function? If yes then it's probably simplest to keep the same API as the official one. If no, then since undefined is not an allowed value to save in firestore anyways I think it could be used as a placeholder for when the document reference does not point to a defined document :)

samuelgozi commented

@sesam to be honest I don't like much the official API, and I think it can be improved, so regarding your question about deferring the parsing of the data, I don't think its a good API design.
As for returning undefined, I think I will go with that instead of throwing an error mainly because I believe most devs will prefer it that way.

  • 👍4