Skip to content

Conversation

@MatthewFallon
Copy link

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

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

@MatthewFallon sorry we were slow to respond here.

initialData's type is any:

initialData?: T | any;

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

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

@MatthewFallon MatthewFallon deleted the matthewfallon/use-observable-fix branch May 14, 2022 01:15
@FirebaseExtended FirebaseExtended locked and limited conversation to collaborators Jun 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants