Open ky28059 opened 2 years ago
@jhuleatt If you would like, you can assign this to me. I would be happy to implement this. I was thinking this could be an option like: allowNullReference
. This would prevent breaking anyone using this feature to their advantage, and also allow people to enable this functionality.
Thank you!
Hey @ky28059 and @epodol, this request has come up a couple of times (https://github.com/FirebaseExtended/reactfire/issues/178#issuecomment-550503600, https://github.com/FirebaseExtended/reactfire/issues/166#issuecomment-544642338).
My opinion is that this would create an opportunity for subtle bugs, and is better handled by breaking things out into separate components (render one component if someone isn't signed in, render a different one that does a Firestore query once you know who the user is).
I wrote out an in-depth response here (note that it's an old version of ReactFire): https://github.com/FirebaseExtended/reactfire/issues/178#issuecomment-551283156. Please take a look at that thread. If you still disagree and can provide a specific code sample that shows why breaking out into different components is impractical, I'd be open to discussing further.
Hey @ky28059 and @epodol, this request has come up a couple of times (#178 (comment), #166 (comment)).
My opinion is that this would create an opportunity for subtle bugs, and is better handled by breaking things out into separate components (render one component if someone isn't signed in, render a different one that does a Firestore query once you know who the user is).
I wrote out an in-depth response here (note that it's an old version of ReactFire): #178 (comment). Please take a look at that thread. If you still disagree and can provide a specific code sample that shows why breaking out into different components is impractical, I'd be open to discussing further.
This actually makes a lot of sense, and I agree with you now. Thank you for your explanation!
Hi there - thanks for the very helpful lib @jhuleatt.
Just jumping in as there is actually a use-case where breaking into small components might not be convenient. This happens whenever you want to feed a component with remote default values if they exist, I think it is actually a quite common use-case.
Just to illustrate simply this use-case, say we have a public contact form page.
In this very case, breaking into small components might not be very practical as we need to create dedicated wrapper component only to fetch the data and populate the default values.
const Form = ({defaultValue}:{defaultValue?: User}) => {
return (
<form>
<input defaultValue={defaultValue.name} disabled={!!defaultValue}/>
<form>
)
}
const FormWithDefaultValue = ({uid}:{uid: string}) => {
const userRef = useFirestore().collection('users').doc(uid)
const { data } = useFirestoreDoc(userRef);
return (
<Form defaultValue={data}>
)
}
const ContactPage = () => {
const {data: signInCheckResult} = useSigninCheck()
if (signInCheckResult.signedIn) {
return <FormWithDefaultValue/>
} else {
return <Form/>
}
}
Happy to get your views, thanks.
(Disclaimer: I might not yet very familiar with ReactFire, please forgive me if I miss understood its usage philosophy).
Hi @jhuleatt , I have read your comments in #178. I do however believe that there is still a necessity for this.
Specifically, in our codebase, we have a number of documents referencing each other i.e.
type Page = {
id: string,
theme: DocumentReference(Theme) | null,
view?: DocumentReference(View) | null,
user?: DocumentReference(User) | null,
}
Rendering requires getting a number of documents which is not easily broken down into subcomponents. In our case it would require nesting a number of levels deep.
Having a nullable or undefined argument will greatly simplify things. We do have undefined and null in javascript to distinguish to enable the API to distinguish between not yet resolved and not available.
useFirestoreDoc(null)
can still return a doc but with doc.data() == null
and doc.exists() == false
Any updates on this issue as Im currently facing a similar problem
Also facing this same issue. In my situation, the user id is required to make the query. Breaking apart things into components for the sake of this current limitation would be incredibly inefficient.
Any updates? I am almost changing the library cause this does not make sense to me.. I had a lot of places already that rules of hooks are being broken cause I need conditionals..
For example in Next.js I can't init performance cause I need to check if it's on the browser and everything is a hook and the rules are broken..
So, I have a few ideas on how to fix this:
For now to be honest I will have to change library for Firebase..
Unfortunately changing the library as well as this is a deal breaker
Current behavior leads to code like this all throughout my codebase:
const { data: networkMemberships } =
useFirestoreCollectionData<NetworkMembership>(
profile
? query(
collection(network.ref, "members").withConverter(
converter<NetworkMembership>()
),
where("profileRef", "==", profile.ref)
)
: collection(network.ref, "noop").withConverter(
converter<NetworkMembership>()
),
{ idField: "id" }
);
😢
I would love for this to be more like:
const { data: networkMemberships } =
useFirestoreCollectionData<NetworkMembership>(
profile &&
query(
collection(network.ref, "members").withConverter(
converter<NetworkMembership>()
),
where("profileRef", "==", profile.ref)
)
);
In react-query there is enabled: true
which works perfectly for all these use cases.
So with reactfire you would do:
const { data } = useFirestoreDocData<User>(doc('users/' + userId), { enabled: !!userId }));
The type of data
would be User | null
.
Or with collections:
const { data } = useFirestoreCollectionData<User[]>(query(collection('users')), { enabled: !!userId });
The type of data
would be User[] | null
.
Any update on this issue? Its been almost 3 years 👴🏿
react-firebase-hooks
allows the document reference touseDocument
to be nullable and just returns undefined if it is. This was useful for paths relying on nullable info, likeauth.currentUser.uid
:where if the user were not signed in there wouldn't be an error thrown since
useDocument
would just returnundefined
. It would be nice if a similar feature were added to reactfire, aswon't work as the type of ref is
DocumentReference
, notDocumentReference | null
,will throw an error when the user is not signed in (as the path will become invalid), and
violates the rules of hooks. Currently relying on a rather abhorrent workaround to resolve this and it would be ideal if reactfire could support this behavior natively.