feat(firestore): support waitForPendingWrites() API #4176
Conversation
|
|
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/invertase/react-native-firebase/h5qee7crb |
There was a problem hiding this comment.
This is excellent, thank you!
One quick question in the code about cancellation vs error
Then in general:
Checking upstream docs,
-
android: this was first included here https://firebase.google.com/support/release-notes/android#firestore_v21.1.0 which is linked to bill of materials 22.2.0 in Sep 2019 - need to make sure that's mentioned in the change notes and marked breaking for people if dropping support for older versions wasn't already done in an older breaking change I think - will investigate further
-
ios: https://firebase.google.com/support/release-notes/ios#cloud-firestore_24 on Sep 5 2019 in iOS Pod 6.8.0
I think both of those are old enough there is no problem and this is correctly tagged as feature release not breaking - I'll work through that
mikehardy
added
Workflow: Needs Review
…-native-firebase into wait-for-pending-writes
This comment has been minimized.
This comment has been minimized.
|
I've run prettier on the e2e file, hopefully it will pass now. |
mikehardy
added
Workflow: Pending Merge
|
No worries, the repo is really clear on how to do the e2e tests even
thought here are quite a few moving parts.
Cheers,
Mark.
…On Sun, 30 Aug 2020 at 17:27, Mike Hardy ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In packages/firestore/lib/index.d.ts
<#4176 (comment)>
:
> @@ -1969,6 +1969,24 @@ export namespace FirebaseFirestoreTypes {
* ```
*/
clearPersistence(): Promise<void>;
+ /**
+ * Waits until all currently pending writes for the active user have been acknowledged by the
+ * backend.
+ *
+ * The returned Promise resolves immediately if there are no outstanding writes. Otherwise, the
+ * Promise waits for all previously issued writes (including those written in a previous app
+ * session), but it does not wait for writes that were added after the method is called. If you
+ * want to wait for additional writes, call `waitForPendingWrites()` again.
+ *
+ * Any outstanding `waitForPendingWrites()` Promises are rejected during user changes.
Fascinating - that is not what I was expecting based on my first read of
the docs - the added test that probes it and the docs are great now, given
that testing result. Can't tell you how much seeing a PR with tests (and
then an iteration, that improves the docs) is just amazing, lends
confidence to releasing it. Thanks
I'll double check the version stuff (just to make sure we don't
accidentally issue a breaking change without a semver major - no problems
either way I just want to get that right, no problem issuing a semver
major...)
This looks great though
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#4176 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABO7LXLNC3GOTI5CLG3MQDSDJ4V7ANCNFSM4QPUHALA>
.
|
|
dependency analysis completed - there was already a breaking change that bumped the upstream sdk dependencies past the ones needed here: #3613 I will note in the release notes that this requires firebase-android-sdk bill of materials >= 22.2.0 and firebase-ios-sdk pod >= 6.8.0 for the avoidance of doubt, but it does not need a semver major However, the e2e test is failing in CI for some reason, both android and ios look like this for the failure 🤔 |
Ouch ... I re-verified the iOS test locally and it is passing for me - I don't suppose there are any more verbose logs saying which line it failed on? Is it working for you? |
|
Hmm There are not more verbose logs unless you actually add them (temporarily) to spit out console.log etc during the test. The android emulator and ios simulator both generate artifacts (available middle-top-right of the workflow run) where you can see things spit out by the emulator or simulator but I will be very surprised if those contain the info that points to the problem - those are mostly used by me to diagnose (and fix) CI flakiness issues where the virtual devices are mis-behaving or having timing issues etc I have not attempted to reproduce locally - I was hoping you'd see some obvious thinko on inspection and post a fix 😅 - if you don't see anything, I'd propose instrumenting heavily - every single statement get a console.log stating where you are during execution then hopefully we can pinpoint it after pushing that and letting the test run This part may be frustrating |
|
Unrelated flake on the Android E2E test but iOS still failed with that change 🤔 |
| try { | ||
| await firebase.auth().signOut(); | ||
| } catch (e) {} |
There was a problem hiding this comment.
That seems like it was it!
|
Fully green in CI now, thanks for sticking with that! Terribly slow trial-and-error fix process when it's a CI only break... |
mikehardy
removed
Type: In Progress
Workflow: Needs Review
|
And it's live - thanks!
|
* Implement waitForPendingWrites * Remove spurious extra typedoc doc open * Added test * Ran prettier and added test of rejection on user change * Made documentation clearer Note: This requires firebase-android-sdk bill of materials >= 22.2.0 and firebase-ios-sdk pod >= 6.8.0 Co-authored-by: Mike Hardy <github@mikehardy.net>
Description
Support waitForPendingWrites() API
Release Summary
Checklist
AndroidiOSe2etests added or updated inpackages/\*\*/e2ejesttests added or updated inpackages/\*\*/__tests__Test Plan
Think
react-native-firebaseis great? Please consider supporting the project with any of the below:React Native FirebaseandInvertaseon Twitter