firebase / firebase-js-sdk

Firebase Javascript SDK
https://firebase.google.com/docs/web/setup
Other
4.86k stars 891 forks source link

Firestore in v9: Network error occurs despite using .catch #5302

Closed matthew-e-brown closed 2 years ago

matthew-e-brown commented 3 years ago

Describe your environment

Describe the problem

When attempting to update a single field on a document, the updateDoc function is used. If the requesting document does not exist, that obviously means it can't be updated, and instead must be set. Personally, I do this by catching the updateDoc's error and then delegating to setDoc. In Firebase v8, there were no issues and things went just fine. However, in the v9 beta, despite catching it, the console logs an error:

Error message in console that says, "GET (url) net::ERR_FAILED 200"

This error also appears in the Network tab with a status of, "(failed) net:ERR_FAILED", but, as hinted at from the error message, it shows a status of HTTP 200 OK, with nothing my eyes can find throughout the request/response headers that looks to be amiss.

Peculiarly, this error only occurs in Chromium browsers (Chrome and the new Edge); Firefox seems to catch it just fine.

Steps to reproduce:

  1. Attempt to update a non-existent document.
  2. Catch the error that comes from doing so.
  3. Set the document instead.
  4. Network error appears in console.

As part of making this bug report, I thought it would be helpful to make a minimal reproducible example for whomever sees this. The project I was working on when this came up was a bit large to comb through, so I replicated the setup I was using (Vue 3, TypeScript, Webpack) and put it in a repo. It's available at https://github.com/matthew-e-brown/firestore-bug-mre. The relevant code is in /src/cloud.ts.

Relevant Code:

const docRef = doc(firestore, path);

// Only update the particular field we provide, not the whole document
const data = { [key]: value };

// If it's not found, set a new document that has the single field set on it
return updateDoc(docRef, data).catch(() => setDoc(docRef, data));
//     ^^^^^^^^^               ^^^^^
//     error from this         despite being caught
//     failing is still        and handled over here
//     logged in console

Other notes

Clicking on the filename in the console error, it jumps to some bundled index.esm.js file with this line highlighted:

k.send = function (a) { if (1 != this.readyState)
    throw this.abort(), Error("need to call open() first. ");

After some Googling and searching GitHub, I found that this line comes from the Closure Library: https://github.com/google/closure-library/blob/7c5e8ef152adf9cc814875c42ab2a0244653b69c/closure/goog/net/fetchxmlhttpfactory.js#L253-L256. Somehow I doubt that the Closure Library is causing this issue, but I found it interesting nonetheless, and I don't think including extra details could hurt, so I'm leaving this note at the bottom.

matthew-e-brown commented 3 years ago

Oh, I should mention that, as far as I can tell, everything still seems to function properly. The update fails, the catch runs, and the setDoc works; Firestore gets updated with new data. It's just that something, somewhere, is spitting out an error into the console. Even if it works, it's probably still a problem somewhere.

dconeybe commented 3 years ago

Hi @matthew-e-brown. Thank you for the bug report. The minimal reproducible example is excellent and I was easily able to reproduce the misleading error message with it in Chrome, and confirmed that the same error message is not present in Firefox. I'll investigate and reply back with my findings.

dconeybe commented 3 years ago

After further investigation and discussion with the team, I can happily confirm that the net::ERR_FAILED 200 error can be safely ignored. It is a side effect from the v9 API changing to use Fetch instead of XMLHttpRequest. I've filed a bug against the WebChannel team (b/196828044 for Googlers) to investigate. I have no ETA for a fix but I wouldn't expect a prompt fix since there are no behavioral issues (i.e. the desired behavior is still observed) and it will likely be prioritized as such. I'm going to leave this issue open until there is a concrete resolution.

In the meantime, there are two options that come to mind to handle this:

  1. Ignore it - The error is not harmful to your app and can be safely ignored, albeit annoying and potentially misleading for anyone reading the console output.
  2. Use XMLHttpRequest - You can also "downgrade" Firestore to use XMLHttpRequest instead of Fetch (instructions below). The downside is that XMLHttpRequest is not supported in some environments (like service workers).

To use XMLHttpRequest instead of Fetch, specify useFetchStreams:false when initializing Firestore. For example, in the app you provided, that would be done by changing the line

export const firestore = getFirestore(firebaseApp);

to

export const firestore = initializeFirestore(firebaseApp, {useFetchStreams:false} as any)

and replacing the import of getFirestore with initializeFirestore.

I confirmed that making this change caused the net::ERR_FAILED 200 error to go away.

I hope this analysis helps. Please let me know if you have other questions or comments about this issue. Again, thanks for providing such a detailed and complete bug report, especially the reproduction app. It made investigation as quick as it could have been.

matthew-e-brown commented 3 years ago

Hi @dconeybe thanks for your just-as-detailed response! 😄

XMLHttpRequest is not supported in some environments (like service workers).

The app I'm working on is going to have PWA support, so I'll be using a service worker. I guess I've got no choice but to put up with it for now. Glad it's been forwarded to the right team. Cheers. 😃

dconeybe commented 2 years ago

Update: A fix has been submitted to Chromium: https://chromium-review.googlesource.com/c/chromium/src/+/3512334 and hopefully it will be available in May or June 2022. Even then, it will probably be a while until all of your users have browsers that include the fix.

Just to reiterate, the error message is spurious and can be safely ignored. The "fix" merely avoids the situation that resulted in emitting the spurious et::ERR_FAILED 200 error message, and doesn't change any logic.

I'm going to close this issue since there are no further actions from Firestore, but feel free to request that it be re-opened if you are not satisfied with this resolution.

dconeybe commented 2 years ago

Update: The bug is fixed in Chrome Canary Version 102.0.4956.0 (Official Build) canary (x86_64).