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

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

Closed
KoryNunn opened this issue Aug 5, 2022 · 17 comments · Fixed by #6899
Closed

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

KoryNunn opened this issue Aug 5, 2022 · 17 comments · Fixed by #6899

Comments

@KoryNunn
Copy link
Contributor

KoryNunn commented Aug 5, 2022

[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: #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 KoryNunn changed the title enablePersistance in an iOS WebView causes quries to take minutes to resolve. enablePersistance in an iOS WebView causes queries to take minutes to resolve. Aug 5, 2022
@KoryNunn
Copy link
Contributor Author

KoryNunn commented Aug 5, 2022

After further testingI'm almost certain the issue is due to the following fix not being released to version 8.X of the sdk:

if (isSafari() && navigator.appVersion.match(/Version\/1[45]/)) {

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

@KoryNunn
Copy link
Contributor Author

KoryNunn commented Aug 5, 2022

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

@dconeybe dconeybe self-assigned this Aug 5, 2022
@dconeybe
Copy link
Contributor

dconeybe commented Aug 5, 2022

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?

@KoryNunn
Copy link
Contributor Author

KoryNunn commented Aug 7, 2022

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.

@dconeybe
Copy link
Contributor

dconeybe commented Aug 8, 2022

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"?

@google-oss-bot
Copy link
Contributor

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!

@KoryNunn
Copy link
Contributor Author

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

@dconeybe
Copy link
Contributor

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?

@KoryNunn
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@dconeybe
Copy link
Contributor

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).

@KoryNunn
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@dconeybe
Copy link
Contributor

@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
Copy link
Contributor

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?

@KoryNunn
Copy link
Contributor Author

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:

#6899

@dconeybe
Copy link
Contributor

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

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