search

FirebaseExtended / reactfire

Hooks, Context Providers, and Components that make it easy to interact with Firebase.
https://firebaseopensource.com/projects/firebaseextended/reactfire/
MIT License
3.51k stars 399 forks source link

Fix for evaluating initialData of observable #507

Closed MatthewFallon closed 2 years ago

MatthewFallon commented 2 years ago

Changing the useObservable method to evaluate initialData as falsy rather than using hasOwnProperty which accidentally returns "success" early from an observable.

Description

CLA signed for this account.

Relevant issue: #495

Tests for useObservable still passing. I did not get the emulators set up properly for some of the additional tests on my own machine.

Currently your docs state that useUser should be returning "loading" until it emits it's first value, however you are accidentally interpreting initialData: undefined | null as equal to true thus returning the wrong status on first render. This fixes that. I simply changed one line to make it so that it evaluates the falsy value of the initialData coming in rather than the existence of the property, so that it does not have false positives like this in the future. I am not certain if this could cause other unintended consequences, I would appreciate if a maintainer has time to take a look.

Code sample

jhuleatt commented 2 years ago

@MatthewFallon sorry we were slow to respond here.

initialData's type is any:

https://github.com/FirebaseExtended/reactfire/blob/eb1428b9e7923a898cff73ca4725c012b4074bae/src/index.ts#L26

Because of this, undefined and null technically are valid values. null is especially important, because that's what the Firebase Auth SDK returns from auth.currentUser when someone isn't signed in.

We could consider ignoring undefined for initialData, but that would be a breaking change that would require us to change the type of initialData. If it's something you feel strongly about and would like to create an issue to discuss for ReactFire v5, we can definitely consider it! Otherwise we should stick with the fix in #521.

MatthewFallon commented 2 years ago

Ok, that definitely makes sense, I didn't even consider the impact it would have on if a user wasn't signed in at all for some reason. Thank you for dropping a reply I appreciate it. :+1: