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

Don't splat classes if using data converters #33

Merged
merged 4 commits into from
Sep 9, 2021
Merged

Conversation

jamesdaniels
Copy link
Collaborator

@jamesdaniels jamesdaniels commented Sep 8, 2021

If one used Firestore data converters & built classes, these were getting thrown away when using docData/collectionData.

class Foo {
  static fromFirestore(snapshot: QueryDocumentSnapshot) {
     return new Foo(...);
  }
}

const fooCollection = collection(firestore, 'foo').withConverter(Foo);

// NOW:
collectionData(fooCollection) //=> Observable<Foo[]>

// BEFORE:
collectionData(fooCollection) //=> Observable<{ ... }[]>

For now I've set use with idField etc. to throw, as there isn't really a point; since one could consume snapshot.id in the converter. In thinking about it I decided to take a less opinionated approach here, a lighter touch for semver for folk using converters now.

Technically speaking this is a break, since if you converter returns a non object it won't be splatted any longer. E.g, undefined will return undefined rather than {} if you use collectionData. But I consider this undefined behavior as we don't provide any guides here, WDYT @jhuleatt?

Follow on:

  • going to dry everything up
  • add generic support (without forcing converters)
  • flush out the tests a bit more

Misc.:

  • Bump versions
  • Add tests back into canary/release script
  • Addressed some flaky tests
  • Yarn.lock was showing in the github diff, mark as generated

@jamesdaniels jamesdaniels marked this pull request as ready for review September 8, 2021 17:04
@jhuleatt
Copy link
Contributor

jhuleatt commented Sep 8, 2021

Technically speaking this is a break, since if you converter returns a non object it won't be splatted any longer. E.g, undefined will return undefined rather than {} if you use collectionData. But I consider this undefined behavior as we don't provide any guides here, WDYT @jhuleatt?

I agree with not calling this a break because we don't currently document how converters that return undefined are handled. We can even say that returning something other than exactly what the converter returned is a bug.

That said, another todo item on this PR should probably be to document converter behavior in RxFire's Firestore docs

@jamesdaniels jamesdaniels merged commit 134e497 into main Sep 9, 2021
@jamesdaniels jamesdaniels deleted the data_converters branch September 9, 2021 03:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants