Closed ralrom closed 2 years ago
have the same :( any solutions?
Found one possible fix to apply for this. The useUser method is creating the observable with useObservable and passing it a initialData value of null
, I assume with the expectation that that is equivalent to not passing an initial data value... BUT in useObservable the method is using Object.hasOwnProperty to check for the existence of the initialData prop and hasOwnProperty doesn't check for the falsy value of a property it checks for existence.
Quote code from MDN here:
let example = {};
example.hasOwnProperty('prop'); // false
example.prop = 'exists';
example.hasOwnProperty('prop'); // true - 'prop' has been defined
example.prop = null;
example.hasOwnProperty('prop'); // true - own property exists with value of null
example.prop = undefined;
example.hasOwnProperty('prop'); // true - own property exists with value of undefined
This creates the situation where if the property was ever declared, even if not defined, the observable will be told to return success rather than loading. Two options obviously, we can force the useUser function to not define initialData based on falsy or I believe preferably the useObserver method should be fixed to use the result of the value not the hasOwnProperty method to determine initial data, but want to double check if that would have any consequences. Any maintainers in the room that know if there was any specific reason for the choice? otherwise I have a pull request I can put in with the fix. :+1:
Same! any updates??
My current workaround is to check if user is undefined (a logged out user will be null, not undefined)
const { status, data: user } = useUser();
// In addition to checking the loading state, we also check if user is undefined
if(status === 'loading' || typeof user === 'undefined') {
return "Loading..."
}
if (!user) {
return "Not logged in!"
}
return "Hello user!"
Hey all, I was able to repro this today. Here's my theory:
useUser
tries to be clever and skip loading states if the auth.currentUser
object has something: https://github.com/FirebaseExtended/reactfire/blob/823eaa2e66098a9c68909aa6676932e3422dc6bf/src/auth.tsx#L28-L35
I thought this was ok, because auth.currentUser
was undefined
if it hadn't loaded, meaning initialData
was undefined. But the core of ReactFire, useObservable
, checks hasOwnProperty
, and I think that means that undefined
would evaluate to true
.
Edit: D'oh, @MatthewFallon said the same just a couple of comments above
The fix for this is available in a canary build: npm install reactfire@4.0.0-exp.eb1428b
Since today is Friday, I'm going to wait to publish it as 4.2.2
until Monday
Yes @jhuleatt as I mentioned in my comment and pull request I dropped earlier, this is definitely what was occurring. Fixing it directly inside the useUser will definitely work as a quick fix.
I did just want to double check if you know if there is any specific reason to use hasOwnProperty instead of simply evaluating by the falsy value of the result as would generally be expected? This might cause other unexpected issues for you all in the library, #507 would simply change that evaluation to the other, is it preferable to leave it as hasOwnProperty? If it's better to leave it alone feel free to close out my PR as well on this :+1:
@MatthewFallon sorry I missed your comment and PR 😞 I'll respond in your PR
Version info
Next: 12.0.7 React: 17.0.2 Firebase: ^9.6.1 ReactFire: ^4.2.1 Suspense mode: off
Test case
Expected Output
I expect status === "loading" for the first render
Actual behavior
I get status === "success" for the first render despite the value still being undefined at that point