-
-
Notifications
You must be signed in to change notification settings - Fork 269
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
(casl-mongoose) fix: Proper typing for Mongoose plugins #732
base: master
Are you sure you want to change the base?
Conversation
* Mongoose made this more strict in 6.6.3 - Automattic/mongoose@2b0d429
3273cbc
to
a1fc7b2
Compare
Codecov Report
@@ Coverage Diff @@
## master #732 +/- ##
=======================================
Coverage 94.81% 94.81%
=======================================
Files 34 34
Lines 733 733
Branches 174 174
=======================================
Hits 695 695
Misses 18 18
Partials 20 20
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
hey, thank you for the fix. did you tested this? I'm asking because not really sure that passing Mongoose team as usually, do breaking type changes in non-breaking releases... |
Hey @stalniy,
Yes - I have just tried it again within an isolated test project and the following code: import mongoose, { Types } from 'mongoose';
import { AccessibleModel, accessibleRecordsPlugin, accessibleFieldsPlugin } from '@casl/mongoose';
interface IPost {
_id: Types.ObjectId;
title: string;
author: string;
}
type PostModel = AccessibleModel<IPost>;
const Post = new mongoose.Schema<IPost, PostModel>({
title: String,
author: String
});
Post.plugin(accessibleRecordsPlugin);
Post.plugin(accessibleFieldsPlugin);
export const PostModel = mongoose.model<IPost, PostModel>('Post', Post); Contrary to what I wrote above, the mentioned compile error happens for me with Mongoose With the changes in the MR, the code as shown above compiles with
Tell me about it - constant frustration 😠 |
@@ -77,7 +77,7 @@ export interface AccessibleRecordModel< | |||
> | |||
} | |||
|
|||
function modelAccessibleBy(this: Model<unknown>, ability: AnyMongoAbility, action?: string) { | |||
function modelAccessibleBy<T>(this: Model<T>, ability: AnyMongoAbility, action?: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no need to make it generic because this types anyway are thrown away
Hi @stalniy -
I continue to enjoy using CASL - thank you for your work! After updating Mongoose in one of our projects I got compile errors here:
Mongoose 6.6.3 made the plugin typing more strict, which currently leads to a TS error which I have to
@ts-ignore
See here about the change:
Automattic/mongoose@2b0d429
These changes add the second type parameter to
Schema
which resolves the issue. The fix should be non-breaking.