GraphQLGuide / apollo-datasource-mongodb

Apollo data source for MongoDB
MIT License
285 stars 64 forks source link

Enforce sane typings on Collection. #22

Closed jblevins1991 closed 4 years ago

jblevins1991 commented 4 years ago

I found the issue I was having yesterday was due to Collections being one of two Typescript types. It can either be the Collections type from Mongoose or the MongoDB native driver. To workaround this, I had to do the following coersion.

createCategory(name: string) {
    return (this.collection as MongoCollection).insertOne({ name });
}

The current type of collection on MongoDataSource is Collection which is determined by if T extends Mongoose's Document type or not. I have not tested this, but I wonder if there is a way to do collection: MongooseCollection | MongoCollection<T>? I have done this in the past to combine redux action types and wonder if it could be applied here or not. I think there is a way to make this type coersion not required. This would be a better developer experience.

9at8 commented 4 years ago

Collections being one of two Typescript types. It can either be the Collections type from Mongoose or the MongoDB native driver.

Correct.

To workaround this, I had to do the following coersion.

If you are specializing MongoDataSource correctly, then there should be no need for it.

For the native driver:

interface Product {
  name: string
  price: number
}

class ProductDataSource extends MongoDataSource<Product> {
  create(product: Product) {
    // As MongoDataSource was specialized with Product,
    //  this.collection will always be MongoCollection.
    //  It can never be a MongooseCollection.
    return this.collection.insertOne(product)
  }
}

Maybe we should be clear about this in the docs :thinking:

I wonder if there is a way to do collection: MongooseCollection | MongoCollection?

I think doing collection: MongooseCollection | MongoCollection<T> would not be wise. Doing that isn't incorrect, but in my opinion, it is not strict enough. My reason for opting for conditional types is to make types stricter and easier to work with. :v:


Maybe I don't understand your use-case. Anyhow, let me know if you have any more questions! :smile:

jblevins1991 commented 4 years ago

I think that we should just provide documentation to highlight that this.insertOne() will always result in a compilation error from the Typescript engine. Therefore, there is always a need to coerce the type.

import { Collection as MongoCollection } from 'mongodb';

class ProductDataSource extends MongoDataSource<Product> {
  create(product: Product) {
    const result = await (this.collection as MongoCollection).insertOne(product);

    // do additional things here
  }
}

The reason why this is an issue is due to this compilation error, not with the .insertOne(), but what you pass to the .insertOne(). Because of the type given to collection on the MongoDataSource class, collection behaves in a way that the native mongo driver would never behave without the coersion, which is why we get the error below.

Argument of type '{ stuff: ObjectID; name: any; description: any; }' is not assignable to parameter of type 'Pick<Collection<{ _id: ObjectId; stuff: ObjectId; name: string; description: string; things: ObjectId[]; }>, "collectionName" | "namespace" | ... 45 more ... | "watch"> & { ...; }'.
  Object literal may only specify known properties, and 'stuff' does not exist in type 'Pick<Collection<{ _id: ObjectId; stuff: ObjectId; name: string; description: string; things: ObjectId[]; }>, "collectionName" | "namespace" | ... 45 more ... | "watch"> & { ...; }'.ts(2345)
9at8 commented 4 years ago

@jblevins1991 I am using it like this in my project and I don't have the error that you're looking at.

I'm using TypeScript v3.8.3, @types/mongodb v3.5.8, apollo-datasource-mongodb v0.2.5

export interface LobbyForm {
  name: string
  maxSize: number
  ownerId: ObjectId
  players: ObjectId[]
}

export type LobbyCollection = WithId<LobbyForm>

export class LobbyDataSource extends MongoDataSource<LobbyCollection, Context> {
  async createLobby({
    name,
    maxSize,
    ownerId,
  }: CreateLobbyForm): Promise<LobbyCollection> {
    const players = [ownerId]
    const form: LobbyForm = {name, ownerId, players, maxSize}

    const validationResult = validateLobby(form)

    if (validationResult !== true) {
      throw new UserInputError('Bad input', validationResult)
    }

    const {
      result: {ok},
      ops,
    } = await this.collection.insertOne(form)

    if (ok !== 1) {
      throw new Error('Something bad happened while creating a lobby')
    }

    return {...form, _id: ops[0]._id}
  }
}
9at8 commented 4 years ago

Closing this as I couldn't replicate the problem. I would be happy to help if there's anything else. :)