firebase-js-sdk icon indicating copy to clipboard operation
firebase-js-sdk copied to clipboard

enablePersistance in an iOS WebView causes queries to take minutes to resolve.

Open KoryNunn opened this issue 3 years ago • 9 comments

[REQUIRED] Describe your environment

  • Operating System version: iOS (All)
  • Browser version: Safari 15.5 (Web View ONLY, eg: cordova)
  • Firebase SDK version: 8.10.1
  • Firebase Product: firestore

[REQUIRED] Describe the problem

After calling enablePersistence, the next query will not resolve for up to 10 minutes. After it resolves, everything seems to work as normal.

~~This looks like a regression of: https://github.com/firebase/firebase-js-sdk/issues/3120~~ More likely to be just un unreleased fix, see first comment.

If enablePersistence is not called, everything works as expected.

Steps to reproduce:

  1. Call firestore.enablePersistance();
  2. Do a .get()

Relevant Code:

N/A

KoryNunn avatar Aug 05 '22 00:08 KoryNunn

After further testingI'm almost certain the issue is due to the following fix not being released to version 8.X of the sdk: https://github.com/firebase/firebase-js-sdk/blob/46fe1362cf9eaac216d4a7bbdc234f472567aa44/packages/firestore/src/local/indexeddb_persistence.ts#L980

If i make that change manually in my node_modules, the app works correctly.

KoryNunn avatar Aug 05 '22 01:08 KoryNunn

Additionally, WKWebView does not report with an appVersion that includes "Version", at least in Cordova, so this check will continue to fail.

KoryNunn avatar Aug 05 '22 06:08 KoryNunn

Hi @KoryNunn. I noticed that you've reported this issue against v8.10.1. We are no longer making releases of the v8 SDK, except in the case of emergency security vulnerabilities (e.g. https://firebase.google.com/support/release-notes/js#version_8101_-_january_28_2022).

Are you able to reproduce this issue in the latest version of this SDK, which is 9.9.1 at the time of writing?

dconeybe avatar Aug 05 '22 16:08 dconeybe

We cannot trivially update to v9 due to how v9 automatically decideds what environment it is running in. This issue will not exist in v9 because the fix has already been added, it's just not in v8.

If the target environment in v9 could be manually spcified we could upgrate without difficulty, but for now, it's a black box that decideds its running in node/web it's self, in our unit tests, which then don't work.

KoryNunn avatar Aug 07 '22 23:08 KoryNunn

Hi @KoryNunn. If you continue using v8 then there isn't anything I can do to help. Your option would be to maintain your own fork with the required patches. This isn't a great long-term solution, though, because the Firestore world will move on in v9 and leave you behind. But it is definitely an option if you so choose.

Since you're blocked from upgrading to v9, then the best way forward would be to focus on that issue. You mentioned "If the target environment in v9 could be manually spcified we could upgrate without difficulty". Could you elaborate? For example, can you clarify what do you mean by "target environment" and what you would need to "specify"?

dconeybe avatar Aug 08 '22 15:08 dconeybe

Hey @KoryNunn. We need more information to resolve this issue but there hasn't been an update in 5 weekdays. I'm marking the issue as stale and if there are no new updates in the next 5 days I will close it automatically.

If you have more information that will help us get to the bottom of this, just add a comment!

google-oss-bot avatar Aug 15 '22 01:08 google-oss-bot

@dconeybe firebase internally looks for things like window or process on global scope, and decideds that it is running in a browser or node. Our integration tests run in node, using jsdom. We can trick v8 into working, but have not succeeded in tricking v9.

If there was a config option or even seperate module for firebase that was spcific to browser/node, we could just tell it "assume you are in a browser" so that it would in our tests.

This isn't fresh in my mind having not attempted an uprade in a while, I'll see if I can have another look soon to be more specific with the issues we run into.

KoryNunn avatar Aug 16 '22 06:08 KoryNunn

Thank you for the information, @KoryNunn. I've changed this issue to a feature request since this sounds like a reasonable thing for the API to expose. I've also logged b/243138668 to keep track of this feature request internally.

Once last question, @KoryNunn: Could you describe the "trick" that you use in v8 that doesn't work in v9?

dconeybe avatar Aug 19 '22 15:08 dconeybe

In our test code that runs in node we:

  1. Add a window property to global, that looks legit (jsdom instance)
  2. Delete the following from firebase:
delete firebaseInstance.INTERNAL.node;

Which causes v8 to think it's running in the browser.

KoryNunn avatar Aug 30 '22 04:08 KoryNunn

Hi, a frustraiting update.

We've once again attempted to upgrade to v9, but run into issue after issue.

We run our ~1100 integration tests in node, but v9 attempts to be helpful and "know" what version of the code to run via a package.json exports. This causes our tests to load firebase for node, but we need the browser version.

If firebase just had simple environment-based files/modules, like @firebase/app/browser without making assumptions about how it would be consumed, we would have no issues.

We also run into issues because the dist files are esm modules. We use require because it's the most robust way to import different dependencies (and likely will be for the forseable future given the last decade worth of javascript). We do not build our test code before running, and we should not be forced to re-artchitect our project just because one dependency is built in a special way.

I realise this is likely to be ignored, but my advice is to let the consumers of code choose how to consume it, and write modules in such a way that they can be consumed as desired. The way v9 is built rail-roads a particular usage pattern. Using the decade-old require and module.exports pattern is trivially transpilable to the shiny-new backwards-imcompatible ESM imports syntax, but doesn't dictate how consumers must artchitect their project.

We are stranded between v8, with many never-to-be-fixed bugs, and v9, which is evidently going to be a huge enineering cost to support.

KoryNunn avatar Sep 23 '22 02:09 KoryNunn

Thank you for the update. I'm sorry that your experience upgrading to v9 is giving so much trouble. One update that I have is that we are actively looking into ways to override the auto-detection of node vs. browser in v9 (googlers see b/242045235). Unfortunately, I don't have an ETA for the feature to actually get released, nor a workaround in the meantime.

As for the packaging issues, I'll pass along your feedback to the people who own the larger structure of the repo (I, personally, only work on the Firestore component).

dconeybe avatar Sep 23 '22 02:09 dconeybe

We have updated to v9 at great time-cost.

This bug is still present. In cordova, the appVersion does not contain "Version/". Making this a manually configurable option, rather than or in addition to doing UA sniffing would allow this to be resolved.

Our current solution involves overwriting our UA string so that firestore detects /Version/1[45]/ and doesn't hang our app.

KoryNunn avatar Nov 29 '22 08:11 KoryNunn

Also, appVersion is deprecated: https://developer.mozilla.org/en-US/docs/Web/API/Navigator/appVersion

KoryNunn avatar Nov 29 '22 08:11 KoryNunn

@KoryNunn Thanks for the update. My update is that work on the manual overriding of the node/browser detection is still a work in progress; however, it's taking a while due to competing priorities and some unanticipated technical challenges. Although I can't promise anything, I hope to be able to share something more concrete in Jan-Feb 2023.

dconeybe avatar Nov 29 '22 20:11 dconeybe

Update: A new feature to allow users to specify their environment as "node" or "browser" to override Firebase's runtime environment detection and force the SDK to act as if it were in the requested environment was released in v9.16.0 (January 19, 2023): https://firebase.google.com/support/release-notes/js#version_9160_-_january_19_2023.

Based on previous comments, it appears that you have already upgraded your app to v9 of the firebase sdk and must have worked around the browser/node environment detection issues that were blocking your upgrade. In any case, this new mechanism may make your life easier.

Now that you're on v9, are you still seeing issues with queries taking minutes after enabling persistence?

dconeybe avatar Feb 16 '23 20:02 dconeybe

Thanks @dconeybe,

Now that you're on v9, are you still seeing issues with queries taking minutes after enabling persistence?

Yes, this is caused by how firebase detects the environment it's running in to decide whether it needs to work around a bug in Safari:

We have updated to v9 at great time-cost.

This bug is still present. In cordova, the appVersion does not contain "Version/". Making this a manually configurable option, rather than or in addition to doing UA sniffing would allow this to be resolved.

Our current solution involves overwriting our UA string so that firestore detects /Version/1[45]/ and doesn't hang our app.

I have a PR open that takes steps to address this issue:

https://github.com/firebase/firebase-js-sdk/pull/6899

KoryNunn avatar Feb 16 '23 22:02 KoryNunn

FYI the fix has been released in version 9.18.0 on March 16, 2023: https://firebase.google.com/support/release-notes/js#version_9180_-_march_16_2023

dconeybe avatar Mar 23 '23 14:03 dconeybe