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

Add FirestoreDocumentReferable, Add FirestoreQueryResult

Open chFlorian opened this issue 4 years ago • 4 comments

Discussion

  • This is a continuation & rewrite of #8614
  • This PR introduces a wrapper around the query results that allows for easier error propagation and allows to add and delete documents directly through the wrapper

Example

image

API Changes

  • This PR introduces two new types: FirestoreDocumentReferable and FirestoreQueryResult
  • FirestoreDocumentReferable is a protocol that allows to automatically assign the documentID of a fetched document to the created object
  • FirestoreQueryResult is a new wrapper type for the results of a FirestoreQuery. It conforms to Collection and RandomAccessCollection, meaning that the user can (still) directly iterate over the results of the query, while also having access to potential errors and gaining the ability to add or delete documents directly from the Property Wrapper

TODO

  • [x] Integrate Result<[T], Error> into FirestoreQueryResult, making the old specific overload obsolete
  • [ ] Clean up and remove both old overloads of FirestoreQueryObservable (T == [U] and T == Result<[U], Error>)
  • [x] Update the sample app

Pinging people who worked on the initial Property Wrapper implementation: @peterfriese @paulb777 @ncooke3 @mortenbekditlevsen

chFlorian avatar Oct 19 '21 07:10 chFlorian

TODO

  • [ ] Integrate Result<[T], Error> into FirestoreQueryResult, making the old specific overload obsolete
  • [ ] Clean up and remove both old overloads of FirestoreQueryObservable (T == [U] and T == Result<[U], Error>)
  • [ ] Update the sample app

To facilitate the API review, it will be useful to see a before/after example. I suggest to

  1. Not remove the existing overloads quite yet
  2. Update the sample application by adding one or more screens that exercises the new API, ideally using the same collections and documents we've used for the current API.

This will help us see the differences and experiment with the sample app to get an understanding of how the proposed API feels like.

Once we get feedback from the API review, we can decide how to proceed with the overloads.

peterfriese avatar Oct 19 '21 20:10 peterfriese

I didn't get to work on this over the last week, but here's a small progress update:

  • theoretically everything works
  • the custom Collection implementation/conformance crashes a SwiftUI ForEach when going from n to 0 items
  • I started work on an example project to showcase the difference to the "old" implementation

chFlorian avatar Oct 27 '21 19:10 chFlorian

  • the custom Collection implementation/conformance crashes a SwiftUI ForEach when going from n to 0 items

Flo and I had a chat offline today, and discussed the following options to drive this PR along:

  1. Run the current version against one of the latest (pre-)releases of SwiftUI to see if this has been fixed in the framework [AI: Peter]
  2. Build a MCVE (essentially a List view and a custom RandomAccessCollection) to explore if this approach is possible. If we run into the same issues, file a Feedback with Apple and make the MCVE available (via this repo: https://github.com/peterfriese/AppleFeedback) [AI: Peter]
  3. Try implementing a type-constrained extension on Array and explore if we can somehow make the property wrapper's Firestore instance available via the SwiftUI environment [AI: Flo]
  4. If all this fails, implement the missing CRUD features on top of the property wrapper's projected value. [pending result of all of the above]

Our goal is to have a shippable implementation by the end of Q1.

peterfriese avatar Jan 05 '22 15:01 peterfriese

Please close, merge, or comment. We plan to close stale PRs on November 28, 2023.

paulb777 avatar Nov 14 '23 00:11 paulb777