Skip to content
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

fix: Always use lean Mongoose documents #102

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jonasmeier1212
Copy link

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.

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

@lorensr lorensr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

image

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.

index.d.ts Outdated
export interface Options {
ttl: number
}

export class MongoDataSource<TData, TContext = any> extends DataSource<
export class MongoDataSource<TData extends MongooseDocumentOrMongoCollection<any>, TContext = any> extends DataSource<
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lorensr lorensr mentioned this pull request Aug 13, 2022
@jonasmeier1212
Copy link
Author

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
Copy link

doronrk commented Aug 31, 2022

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
Copy link

doronrk commented Aug 31, 2022

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
Copy link

doronrk commented Aug 31, 2022

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
Copy link

doronrk commented Sep 7, 2022

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
Copy link
Member

lorensr commented Sep 12, 2022

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

@doronrk
Copy link

doronrk commented Sep 12, 2022

@lorensr do you have a contribution guide anywhere?

@lorensr
Copy link
Member

lorensr commented Sep 12, 2022

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

@doronrk
Copy link

doronrk commented Sep 19, 2022

@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
Copy link
Member

lorensr commented Oct 8, 2022

@doronrk passing on master for me:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants