Some API suggestions

[0x80]

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 anys 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:

• Prefix boolean values and functions returning a boolean with is* has* etc.. For example active -> isActive, fetching -> isFetching. Not doing so makes names ambiguous and code harder to read IMO.
• ready -> whenReady or something
• If you're not happy or comfortable with Flow yet, maybe try Typescript instead? I love it. Especially in combination with VSCode.
• I didn't manage to write type definitions for the getter/setter functions, but in the end I think having them as plain functions actually makes the API clearer.
• You're not using _ consistently on all private class members, and they are not marked private in the type declaration, only via comments. I realise that _ is a convention from C++ and other languages, but I assume that Flow has a keyword for it. Typescript has private to mark class members. Is there a reason for not using that?
• I would drop the Collection and Document classes all together and implement them as factory functions with private members accessed via a closure. This would get rid of this al over the place. Of course this is very subjective, and I guess if you are used to classes you don't mind this everywhere, but to me the API feels a little java-y.

Lastly I was surprised to find createTime, readTime and updateTime. 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 using FieldValue.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
👍

[IjzerenHein]

Hi, some updates on this:

• active has been renamed to isActive
• fetching has been renamed to isLoading + slight behaviour change to fix Side effects like changing state are not allowed at this point #18
• Document.createTime, readTime and updateTime have been removed as they are no longer supported by firestore document snapshots

[0x80]

@IjzerenHein not sure how to apply the new isLoading prop. If I use it without checking for data, my render function tries to render without data first, so I now do a double check which seems very verbose:

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 isActive useful for?

[IjzerenHein]

@0x80 I'm not sure I understand. And I don't understand the this.user.data check either. .data always returns an object, when it has no data, it returns an empty object {}.

isActive indicates whether the collection or document is actually listening for real-time updates. Typically you don't need this, but it can be useful for debugging.

[0x80]

@IjzerenHein If document .data should always return an object then I think I might have encountered a bug, because it definitely returns undefined for me in some cases. I'm still trying to figure out what the exact circumstances are...

[0x80]

@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]

@0x80 I'd like to close this issue, is that ok?

I've created a separate issue for the case where it's not possible to detect whether the document is empty or has not been fetched (or doesn't exist), #54

[0x80]

@IjzerenHein yes that's fine with me! I hope to find some time soon to get back into all this.