search

tnc-ca-geo / animl-api

Backend for https://animl.camera
4 stars 0 forks source link

Make sure we're following Mongoose Typescript best practices #221

Open nathanielrindlaub opened 1 week ago

nathanielrindlaub commented 1 week ago

There are a bunch of things I don't understand about how Typescript works with Mongoose, but we should read up the best practices and be sure we're implementing it correctly. Here are a few points of confusion:

  1. When to use the HydratedDocument<>generic? From what I can tell from the docs, HydratedDocument provides the document properties as defined in the Schema/document type, as well as all of the standard Mongoose properties/methods. So my guess is any time we are using a Mongoose method like SomeDocument.save() we'll need to be dealing with a HydratedDocument type. If we're just returning the document without using any of it's methods we probably don't need to use HydratedDocument, but it probably wouldn't hurt, either. We obviously use .save() in lots of places, but I'm not sure what other Mongoose document methods we are using and where, but it would be worth auditing all of the model files to make sure we have full HydratedDocument coverage. Maybe we should just use it everywhere we create new documents or fetch them from the MongoDB, even if we're not using those methods?
  2. ObjectId best practices? I have a few questions here: (a) if we don't explicitly set an _id property in a Mongoose Schema and instead rely on Mongoose to add one of type ObjectID for us (e.g. like we do here here), does mongoose.InferRawDocType<SomeSchema> know about the _id field, or do we have to explicitly set it in the schema? (b) Are we using Schema.Types.ObjectId in our schema definitions (as described in here) correctly? I know we use a lot of string _ids, for both good reasons and bad, but I did a search through the code for Schema.Types.ObjectId and only found one instance of it. Is it possible that we only ever explicitly set an id of type ObjectId once in the whole code base?
  3. Subdocument best practices? We use a lot of subdocuments in our schema. I am less sure if we have any "Nested Paths" (subtly different, see subdocument docs), but I wouldn't be surprised if we have a couple instances in there somewhere. I am also not sure if we ever use Mongoose methods directly on subdocuments in the code? I think in most instances when we create or modify subdocuments (for example Image.objects), we then call save() on the Image rather than the Image.object. If that is 100% the case this might not matter to us, but according to the docs, you need do some work to make sure that the returned, hydrated subdocument types have all the regular mongoose document properties and methods.
alukach commented 4 days ago
  1. When to use the HydratedDocument<>generic? ... Maybe we should just use it everywhere we create new documents or fetch them from the MongoDB, even if we're not using those methods?

Yes, I think this pretty much captures it. Any time that we are pulling data out of MongoDB or initialize a model class (e.g new Task({ ... });), that is a hydrated document. To quote the docs you linked:

The User() constructor returns an instance of HydratedDocument<IUser>. IUser is a document interface, it represents the raw object structure that IUser objects look like in MongoDB. HydratedDocument<IUser> represents a hydrated Mongoose document, with methods, virtuals, and other Mongoose-specific features.

The part that I'm admittedly uncertain of is if the _id property will be set on something like new Task({ name: 'foo' });. The typings for a HydratedDocument<> state that it's set, but that's a bit unexpected to me when we havn't yet sent it to the DB. However, that line of thinking comes mostly from my experience with a RDBMS like Postgres where the ID is often generated by the DB itself, not sure if that's how it works in Mongo (I see that there's a the ObjectID() function that generates IDs client-side). 🤷

  1. ObjectId best practices? (a) if we don't explicitly set an _id property in a Mongoose Schema and instead rely on Mongoose to add one of type ObjectID for us (e.g. like we do here here), does mongoose.InferRawDocType<SomeSchema> know about the _id field, or do we have to explicitly set it in the schema? ...

By mongoose.InferRawDocType<...>, do you mean mongoose.InferSchemaType<...>? Either way, it does seem that Mongoose falls a bit short with regards to the presence of _id values on generated interfaces. I opened a similar issue regarding DocumentArray values. It would likely be safer for us to explicitely specify the _id field on all models even though they are automatically added.

  1. ObjectId best practices? (b) Are we using Schema.Types.ObjectId in our schema definitions (as described in here) correctly? I know we use a lot of string _ids, for both good reasons and bad, but I did a search through the code for Schema.Types.ObjectId and only found one instance of it. Is it possible that we only ever explicitly set an id of type ObjectId once in the whole code base?

I'm assuming that you're talking about this blurb:

To define a property of type ObjectId, you should use Types.ObjectId in the TypeScript document interface. You should use 'ObjectId' or Schema.Types.ObjectId in your schema definition.

I think we're safe here, because we don't explicitely specify document interfaces for our schemas. Instead, we use mongoose.InferSchemaType<>.

  1. Subdocument best practices? We use a lot of subdocuments in our schema. I am less sure if we have any "Nested Paths" (subtly different, see subdocument docs), but I wouldn't be surprised if we have a couple instances in there somewhere.

Yes, I also haven't seen any nested paths in the system.

  1. Subdocument best practices? ... I am also not sure if we ever use Mongoose methods directly on subdocuments in the code? I think in most instances when we create or modify subdocuments (for example Image.objects), we then call save() on the Image rather than the Image.object. If that is 100% the case this might not matter to us, but according to the docs, you need do some work to make sure that the returned, hydrated subdocument types have all the regular mongoose document properties and methods.

I find the Mongoose TS docs to be not great. They discuss how to be super hands-on with the type system (eg manually creating interfaces for each schema, passing those to mongoose.Model<...> and new mongoose.Schema<...>), however I have found that we haven't needed to be as hands-on. While the generated types that come from mongoose.InferSchemaType<...> and typeof mongoose.Model<...>(...) aren't perfect, they seem to be pretty good and are allowing us to avoid worrying too much about these issues. So, basically, I think this is one of those "not a problem until it actually proves to be one" kind of situations.