-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
docs: add detailed loadClass() TypeScript usage guide and new loadcla… #15731
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
base: master
Are you sure you want to change the base?
docs: add detailed loadClass() TypeScript usage guide and new loadcla… #15731
Conversation
…ss.test.ts for tsd validation
hasezoey
left a comment
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.
As a first draft this looks good, though i have some style suggestions.
Also @vkarpov15 might it be worth to extend acquit to work with typescript files so that the examples in the documentation actually match the tests?
Co-authored-by: hasezoey <hasezoey@gmail.com>
Co-authored-by: hasezoey <hasezoey@gmail.com>
|
Thanks again for all the detailed feedback @hasezoey! I've just pushed an update that addresses all the suggestions and changes you asked for. I've also fixed those Please let me know what you think when you get a chance! I'm happy to make any other adjustments if needed. |
|
Hi @hasezoey! |
hasezoey
left a comment
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.
Looks better to me, though still some nitpicks.
Co-authored-by: hasezoey <hasezoey@gmail.com>
Co-authored-by: hasezoey <hasezoey@gmail.com>
Co-authored-by: hasezoey <hasezoey@gmail.com>
|
@hasezoey I’ve reviewed everything again and addressed the remaining suggested changes. |
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.
Pull Request Overview
This PR adds comprehensive TypeScript documentation and type tests for using schema.loadClass() with Mongoose, addressing confusion outlined in issue #12813.
Key changes:
- New documentation section in
docs/typescript/statics-and-methods.mdcoveringloadClass()usage patterns,thistyping, and known limitations - New type test file
test/types/loadclass.test.tswith practical examples validating the documented patterns - Clear guidance on manual type merging requirements and caveats with
toObject()/toJSON()/lean()
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| docs/typescript/statics-and-methods.md | Adds comprehensive loadClass() documentation including usage patterns, this typing, limitations table, and complete examples |
| test/types/loadclass.test.ts | Adds TypeScript tests validating loadClass() behavior, type inference, and documenting known limitations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ```ts | ||
| interface MySchema { | ||
| property1: string; | ||
| } | ||
|
|
||
| class MyClass { | ||
| myMethod(this: MyCombinedDocument) { | ||
| return this.property1; | ||
| } | ||
| static myStatic(this: MyCombinedModel) { | ||
| return 42; | ||
| } | ||
| } | ||
|
|
||
| const schema = new Schema<MySchema>({ property1: String }); | ||
| schema.loadClass(MyClass); | ||
|
|
||
| type MyCombined = MySchema & MyClass; | ||
| type MyCombinedModel = Model<MyCombined> & typeof MyClass; | ||
| type MyCombinedDocument = Document & MyCombined; | ||
|
|
||
| const MyModel = model<MyCombinedDocument, MyCombinedModel>( | ||
| 'MyClass', | ||
| schema as any | ||
| ); | ||
| ``` |
Copilot
AI
Nov 10, 2025
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.
Declaration order issue: The code example shows MyClass using MyCombinedDocument and MyCombinedModel types (lines 233, 236) before they are defined (lines 244-246). While TypeScript allows this within the same scope due to type hoisting, this ordering is confusing for documentation purposes. Consider reordering to define the interface and types first, then the class, then the schema and model creation for better clarity.
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.
This is a great point regarding readability.
I explored reordering the declarations (as Copilot suggested), but it unfortunately creates a circular-dependency issue.
The combined types (e.g., MyCombined) depend on the MyClass definition, so they must be declared after the class. If moved before, TypeScript fails because MyClass is not yet defined.
Given that, I think the current order is the most correct.If helpful, I can add a clarifying comment like:
interface MySchema {
property1: string;
}
// 1. Declare the class first.
// It's OK to reference types (MyCombinedDocument) that aren't defined yet
// in 'this' annotations because of how TypeScript handles hoisting.
class MyClass {
myMethod(this: MyCombinedDocument) {
return this.property1;
}
static myStatic(this: MyCombinedModel) {
return 42;
}
}
// 2. Now define the combined types.
// This works because 'MyClass' has been declared.
type MyCombined = MySchema & MyClass;
type MyCombinedModel = Model<MyCombined> & typeof MyClass;
type MyCombinedDocument = Document & MyCombined;
// 3. Continue with the schema and model.
const schema = new Schema<MySchema>({ property1: String });
schema.loadClass(MyClass);
const MyModel = model<MyCombinedDocument, MyCombinedModel>(
'MyClass',
schema as any
);
@vkarpov15 Please let me know what you think — happy to update!
vkarpov15
left a comment
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.
Overall I think something more like the following would be better, @hasezoey WDYT?
interface RawDocType {
property1: string;
}
// `loadClass()` does NOT update TS types automatically.
// So we must manually combine schema fields + class members.
// The model type must include statics from the class
type MyCombinedModel = Model<RawDocType, {}, Pick<MyClass, 'myMethod'>, Pick<MyClass, 'myVirtual'>> & Pick<typeof MyClass, 'myStatic'>;
// A document must combine Mongoose Document + class + schema
type MyCombinedDocument = mongoose.HydratedDocument<RawDocType, Pick<MyClass, 'myMethod'>, {}, Pick<MyClass, 'myVirtual'>>;
const schema = new Schema({ property1: String });
schema.loadClass(MyClass);
// Cast schema to satisfy TypeScript
const MyModel = model<MyCombinedDocument, MyCombinedModel>(
'MyClass',
schema
);Another alternative I'd like to consider is to is recommending using extends on model instances as a way of handling methods, statics, and virtuals. The following works correctly for methods and statics, but has some caveats with virtuals.
class MyClass extends TestModel {
myMethod() {
return 'hello from myMethod ' + this.name;
}
static myStatic() {
return 'hello from myStatic';
}
get myVirtual() {
return 'hello from myVirtual';
}
}
const doc = new MyClass({ name: 'test' });
// All work
console.log(doc.myMethod());
console.log(MyClass.myStatic());
console.log(doc.myVirtual);
// Caveat: output does NOT include myVirtual because myVirtual technically isn't a virtual here
console.log(doc.toObject());Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
hasezoey
left a comment
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.
Looks good to me in its current state as a initial guide. I agree with @vkarpov15's suggestions though.
FYI:
type MyCombinedModel = Model<RawDocType, {}, Pick<MyClass, 'myMethod'>, Pick<MyClass, 'myVirtual'>> & Pick<typeof MyClass, 'myStatic'>;
Those Picks are what i meant with "would require a Filter type", or a lot of manual typing.
|
Thank you @vkarpov15 and @hasezoey for the detailed review!
Everything should be up to date now. Thanks again! |
hasezoey
left a comment
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.
Looks good to me now.
|
Hi @vkarpov15 @hasezoey, pls review this PR when convenient. Thanks! |
hasezoey
left a comment
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.
Still looks good to me
PR Title:
Docs: Add guidance on using loadClass with TypeScript (fixes #12813)PR Description:
Summary
This PR addresses [issue #12813](#12813), which pointed out that it's confusing for developers to figure out how to use
schema.loadClass()correctly with TypeScript.To fix this, I've added a new documentation section to
docs/typescript/statics-and-methods.mdthat provides clear, official guidance.This new section covers the complete pattern discussed in the issue thread, including:
Model<Combined> & typeof Class), and Document (Document & Combined).this: The correct way to type thethisparameter in both instance and static methods.thisparameters aren't supported in getters/setters (ts(2784)).toObject(),toJSON(), or.lean()return plain objects without the class methods, which can lead to runtime errors.To validate this new documentation and prevent future regressions, this PR also adds a new
tsdtest file attest/types/docs-loadClass.test.ts.Examples
The new documentation page itself contains all the code examples:
## Using schema.loadClass()section indocs/typescript/statics-and-methods.md.The new
tsdtest file also serves as a practical, working example of these patterns:test/types/docs-loadClass.test.ts.✅ Adds documentation
✅ Adds type tests
✅ Fixes #12813