fix: Always use lean Mongoose documents

[jonasmeier1212]

With hydrated Mongoose documents they aren't serializable.
Therefore caching can't be used, as it serializes the documents.

See #74 for why this is needed for Mongoose documents.

[lorensr]

Thanks for contributing this! This is breaking, correct? Users with code that depends on receiving a full model will need to change their code after updating?

Looks like npm test is failing:

Ideally we'd also have tests with TypeScript code that use the index.d.ts types (both Mongoose and Mongo) and makes sure it compiles.

[lorensr]

Why MongoCollection? It seems like TData is used for the document type:

    protected collection: Collection<TData>
    protected model: Model<TData>

[jonasmeier1212]

I will drop this part of the code, it is an unnecessary type constraint. I think it was added because of the way we used it internally, but for general usage, it isn't necessary.

[jonasmeier1212]

Yes, it is definitely a breaking change!

I updated the test and the change indicates one of the breaking changes. The lean documents don't have these getters like id, so if the code depends on them it will break it.

What do you mean with tests in Typescript? Rewriting the existing tests in Typescript but leave the rest of the code in plain JS?

[doronrk]

It should be possible to implement this in a way that is backwards compatible. For example, the constructor could take an optional argument that defined a transform function that would be applied to the query result before being written to the cache. One could supply a function that calls toObject and solve this problem in a more generic way.

We have a large codebase, and it's very difficult to be 100% sure that all of our code will still work with .lean being called on every result. It might not be desirable for other clients of this library for other reasons.

tl;dr Why making a breaking change when you can achieve the same end result compositionally, offer greater flexibility, and avoid the breaking change.

[doronrk]

Actually, now that I'm thinking about composition, it seems like the one solution might be to just wrap a cache with a higher order cache which calls toObject in the value passed into set if the value is a mongoose document without requiring changes to this library at all. That's certainly what we would choose to do if the alternative were a breaking change.

[doronrk]

Ah, nevermind. I see the library is already doing _bson.EJSON.stringify(docs) inside so we can't fix this by providing a smarter cache implementation. I'll revert to my original take which is that the best way to fix this is by allowing an arbitrary transform function to be passed, and making this library unopinionated about what transformation is applied to the results of the query before being passed to the cache.

[doronrk]

It should be possible to implement this in a way that is backwards compatible. For example, the constructor could take an optional argument that defined a transform function that would be applied to the query result before being written to the cache. One could supply a function that calls toObject and solve this problem in a more generic way.

@lorensr I'm not sure if I'll have time for this, but if I put up a PR for a solution of this shape would you accept it?

[lorensr]

@doronrk Yeah, a backcompat way to achieve the same goal would be great, thanks ☺️

[doronrk]

@lorensr do you have a contribution guide anywhere?

[lorensr]

@doronrk nope, feel free to ask questions (@lorendsr or [email protected]) or submit a PR with one

[doronrk]

@lorensr , npm test indicates that several tests are failing on master. Is something wrong with my local setup, or is this true for you as well?

[lorensr]

@doronrk passing on master for me: