-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
Some API suggestions #12
Comments
Hi, some updates on this:
|
@IjzerenHein not sure how to apply the new interface AlgoliaUserViewProps {
data: AlgoliaUser;
}
@observer
export default class AlgoliaUserView extends React.Component<
AlgoliaUserViewProps
> {
private user = new Document<User>(`users/${this.props.data.id}`);
private userRegistration = new Document<UserRegistration>(
`userRegistrations/${this.props.data.id}`
);
public render() {
if (this.user.isLoading || this.userRegistration.isLoading) return null;
if (!this.user.data || !this.userRegistration.data) return null;
//... And what is |
@0x80 I'm not sure I understand. And I don't understand the
|
@IjzerenHein If document .data should always return an object then I think I might have encountered a bug, because it definitely returns |
@IjzerenHein the problem occurs when I try to fetch a document from the database that doesn't exist. Then the isLoading prop will be set to false while document.data is missing. Is there another prop that I should check to catch documents that don't exist, or how did you imagine handling this? |
@IjzerenHein yes that's fine with me! I hope to find some time soon to get back into all this. |
As I was going through the code to write out Typescript definitions, I noticed a few things that I think could be improved on the API surface and and types.
First of all there are quite a few instances where a type is declared as
any
. For example the options to the Collection and Document constructors. I don't really see the point in using Flow if these things are not strictly defined.Some of the
any
s can be simply replaced by the Firestore types I think. I put a few extra ones in the ambient declaration posted in #4. There I've also attempted to write out the options interfaces.The query declaration could be based on a function receiving the
ref
as discussed in #11. The ref as it is defined now can also be undefined, which means you have to check if it exists before using it. The example code doesn't guard for this. Which makes me wonder, are you not using Flow when writing that, or is that somehow slipping the Flow type checks currently?If the ref was passed as an argument to the function, I think the calling code could check if ref exists before calling the query function, and then not decide to call the function at all if ref is undefined.
Some other suggestions, which are highly personal and opinionated:
private
to mark class members. Is there a reason for not using that?Collection
andDocument
classes all together and implement them as factory functions with private members accessed via a closure. This would get rid ofthis
al over the place. Of course this is very subjective, and I guess if you are used to classes you don't mindthis
everywhere, but to me the API feels a little java-y.Lastly I was surprised to find
createTime
,readTime
andupdateTime
. As I understand it they are not really meant for public consumption, since they are not indexed by Firestore and therefor can not be used in queries. One of the reasons is that they won't be reliable in offline mode. I read in #9 that you're planning to support that, so I think you might want to reconsider exposing them on the API surface. What use-case do you have for them? I currently write my own timestamps in documents usingFieldValue.serverTimestamp()
.I am not trying to bash your work here. I think the concept is great. I am happy I found it and I'm about to use this in a client project. I hope you find the time to make this library mature. Firestore is still relatively new of course and clearly all of the related NPM modules are in a very early stage still. I am choosing yours mainly because you treat documents similar to how Firestore keeps them, and the API impact on client code is minimal. Also I'm new to MobX but it seems to be a really nice fit for Firestore.
Thanks for listening
👍
The text was updated successfully, but these errors were encountered: