Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sdk 10.3.0 changed firestore webchannel to use xhr instead of fetch #7581

Closed
atsjo opened this issue Aug 23, 2023 · 8 comments · Fixed by #7593
Closed

sdk 10.3.0 changed firestore webchannel to use xhr instead of fetch #7581

atsjo opened this issue Aug 23, 2023 · 8 comments · Fixed by #7593

Comments

@atsjo
Copy link

atsjo commented Aug 23, 2023

Operating System

Windows 11

Browser Version

Chrome/Edge latest

Firebase SDK Version

10.3.0

Firebase SDK Product:

Firestore

Describe your project's tooling

Angular app using esbuild builder

Describe the problem

In sdk 10.2.0 fetch is used for firestore webchannel (I can se that from network tab in devtools, I think it's been used since 9.0), but 10.3.0 changed firestore webchannel to use xhr instead.

Overriding via undocumented useFetchStreams param in initializeFirestore doesn't work anymore either, so guessing this is a bug, and related to #7569. This is supposed to "fix how we enable fetch streams" to correct a bug, but seems to completely disable it. I can observe more aborted/cancelled communication in devtools with xhr, but it's still working...

Steps and code to reproduce issue

upgrade to 10.3.0 and observe communication via devtools

@atsjo atsjo added new A new issue that hasn't be categoirzed as question, bug or feature request question labels Aug 23, 2023
@jbalidiong jbalidiong added needs-attention and removed new A new issue that hasn't be categoirzed as question, bug or feature request labels Aug 23, 2023
@milaGGL milaGGL self-assigned this Aug 23, 2023
@wu-hui wu-hui assigned wu-hui and unassigned milaGGL Aug 23, 2023
@sampajano
Copy link
Contributor

sampajano commented Aug 23, 2023

@atsjo Thanks for the report!

I've taken a quick look — I think with #7569, it is expected that all POST (client -> server) request will now be using XHR, but the GET (server -> client streaming) requests should still be using Fetch. (This was the original intent of the useFetchStreams on the WebChannel side.)

Is that inline with what you have observed also?


Disclaimer: I'm not familiar with Firestore requirements overall so i'm not sure if this is a breaking issue for anyone. I'll let members of the Firebase team comment later.

@wu-hui FYI

@atsjo
Copy link
Author

atsjo commented Aug 23, 2023

in 10.2.0 and earlier both GET and POST for the webchannel uses fetch, but direct requests, like getCountFromServer uses xhr.
image

I am using persistent multitab cache if that matters...

in 10.3.0 both GET and POST for the webchannel uses xhr...
image

@wu-hui
Copy link
Contributor

wu-hui commented Aug 29, 2023

Note the fix does what @sampajano described in #7581 (comment)

"all POST (client -> server) request will now be using XHR, but the GET (server -> client streaming) requests should still be using Fetch."

@maccman
Copy link

maccman commented Aug 30, 2023

@wu-hui Can you elaborate on why it's bad if Firebase uses XHR versus Fetch? Is it just that the XHR API is deprecated?

@wu-hui
Copy link
Contributor

wu-hui commented Aug 30, 2023

Yeah, pretty much. I don't think XHR vs Fetch with requests makes much difference, it's more about we want to use newer APIs as long as it makes sense.

Using Fetch both directions with webchannel's bidi-streams is under-tested however, so we will stay on the default configuration for now, and maybe flip to that later. Again, I don't think there is much to gain here performance-wise.

@atsjo
Copy link
Author

atsjo commented Sep 2, 2023

Tried with 10.3.1, I assume the fix is rolled out there, but its still uses xhr for everything... (both POST and GET). I also observe typically one failed GET when the app starts up (as shown in the screenshot from dev-tools above). I don't care if its using xhr or fetch, just found it strange flipping everything back to xhr after running with fetch for years, with the changelog only mentioning that it changes how it enables fetch... The undocumented useFetchStreams also stopped working, and after running with fetch for years, I would assume using xhr for everything is under-tested, as mentioned above...

@wu-hui
Copy link
Contributor

wu-hui commented Sep 5, 2023

The fix is not out with 10.3.1, should be the one after this.

@atsjo
Copy link
Author

atsjo commented Sep 6, 2023

Verified canary build working as expected, using fetch for web-channel GET requests:-)

@firebase firebase locked and limited conversation to collaborators Sep 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants