-
-
Notifications
You must be signed in to change notification settings - Fork 4k
BREAKING CHANGE: make id a virtual in TypeScript rather than a property on Document base class
#15572
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
Conversation
…erty on Document base class Fix #13079
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 implements a breaking change to make the id property behave as a virtual in TypeScript rather than a property on the Document base class, addressing issue #13079. The change improves developer experience by providing more accurate TypeScript typing that matches Mongoose's runtime behavior.
- Updates TypeScript types to add
idas a virtual toTVirtualsinstead of having it asid?: anyon the Document class - Adds comprehensive test coverage for various
idscenarios including explicit definitions, disabled id option, and type inference - Maintains compatibility with existing schemas that explicitly define an
idproperty
Reviewed Changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/types/virtuals.test.ts | Updates the VirtualsType to include the new id virtual typing |
| test/types/document.test.ts | Adds comprehensive test cases covering different id scenarios and type expectations |
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. Requires changes to typegoose, maybe it would be good to have something like this as a helper type similar to Default__v with inputs of DocType, TVirtuals, TSchemaOptions?
|
What sort of changes does this require for typegoose? The tricky part is that We could add a |
Minor changes, where |
|
Do you have an example of a change you had to make? Would be helpful for me to evaluate the risk of this change. Would adding a |
I think it would help readability, but mostly so that it can be easily plugged into types that wrap
Diff of what i will apply to typegoose laterNote that typegoose will always add diff --git a/src/types.ts b/src/types.ts
index 7791bb71..4e37283a 100644
--- a/src/types.ts
+++ b/src/types.ts
@@ -14,7 +14,7 @@ import type { PropType, Severity } from './internal/constants';
* const doc: DocumentType<ClassName> = await NameModel.create({});
* ```
*/
-export type DocumentType<T, QueryHelpers = BeAnObject> = mongoose.Document<unknown, QueryHelpers, T> &
+export type DocumentType<T, QueryHelpers = BeAnObject> = mongoose.Document<unknown, QueryHelpers, T, DefaultIdVirtual> &
mongoose.Default__v<mongoose.Require_id<T>> &
IObjectWithTypegooseFunction;
/**
@@ -32,7 +32,7 @@ export type ModelType<T, QueryHelpers = BeAnObject> = mongoose.Model<
T, // raw doc type
QueryHelpers, // query helpers
IObjectWithTypegooseFunction, // instance methods
- BeAnyObject // virtuals
+ DefaultIdVirtual // virtuals
>;
/**
* Any-param Constructor
@@ -737,3 +737,5 @@ export type GetFunctionKeys<T extends object> = {
* does NOT filter out getters / setters
*/
export type FilterOutFunctionKeys<T extends object> = Omit<T, GetFunctionKeys<T>>;
+
+export type DefaultIdVirtual = { id: string };
diff --git a/test/tests/types/basicTypegoose.test-d.ts b/test/tests/types/basicTypegoose.test-d.ts
index 73f8c5e3..369e6579 100644
--- a/test/tests/types/basicTypegoose.test-d.ts
+++ b/test/tests/types/basicTypegoose.test-d.ts
@@ -1,7 +1,7 @@
import { expect } from 'tstyche';
import * as typegoose from '../../../src/typegoose';
import { isDocument, isRefType, prop } from '../../../src/typegoose';
-import { BeAnObject, BeAnyObject, IObjectWithTypegooseFunction } from '../../../src/types';
+import { BeAnObject, DefaultIdVirtual, IObjectWithTypegooseFunction } from '../../../src/types';
// decorators return type
expect(typegoose.prop()).type.toBe<PropertyDecorator>();
@@ -121,7 +121,7 @@ async function typeguards() {
// top-level tests
{
if (typegoose.isDocument(someNewDoc)) {
- expect(someNewDoc).type.toBeAssignableWith<typegoose.DocumentType<TypeguardsClass>>();
+ expect(someNewDoc).type.toBeAssignableTo<typegoose.DocumentType<TypeguardsClass>>();
} else {
// this type is currently wrong, typescript cannot remove the case because the input is not restricted enough
expect<unknown>().type.toBeAssignableWith(someNewDoc);
@@ -234,20 +234,23 @@ typeguards();
async function testDocumentType() {
const someNewDoc = new TestClassModel();
- expect(someNewDoc).type.toBe<typegoose.mongoose.HydratedDocument<TestClass, IObjectWithTypegooseFunction & BeAnyObject, BeAnObject>>();
+ expect(someNewDoc).type.toBe<
+ typegoose.mongoose.HydratedDocument<TestClass, IObjectWithTypegooseFunction & DefaultIdVirtual, BeAnObject, DefaultIdVirtual>
+ >();
const someCreatedDoc = await TestClassModel.create();
expect(someCreatedDoc).type.toBe<
- typegoose.mongoose.HydratedDocument<TestClass, IObjectWithTypegooseFunction & BeAnyObject, BeAnObject>[]
+ typegoose.mongoose.HydratedDocument<TestClass, IObjectWithTypegooseFunction & DefaultIdVirtual, BeAnObject, DefaultIdVirtual>[]
>();
const someFoundDoc = await TestClassModel.findOne();
expect(someFoundDoc).type.toBe<typegoose.mongoose.HydratedDocument<
TestClass,
- IObjectWithTypegooseFunction & BeAnyObject,
- BeAnObject
+ IObjectWithTypegooseFunction & DefaultIdVirtual,
+ BeAnObject,
+ DefaultIdVirtual
> | null>();
expect(someNewDoc._id).type.toBe<typegoose.mongoose.Types.ObjectId>();
@@ -268,7 +271,9 @@ async function gh732() {
const doc = await SomeClassModel.create({ someoptionalProp: 'helloopt', somerequiredProp: 'helloreq' });
- expect(doc).type.toBe<typegoose.mongoose.HydratedDocument<SomeClass, IObjectWithTypegooseFunction & BeAnyObject, BeAnObject>>();
+ expect(doc).type.toBe<
+ typegoose.mongoose.HydratedDocument<SomeClass, IObjectWithTypegooseFunction & DefaultIdVirtual, BeAnObject, DefaultIdVirtual>
+ >();
const toobj = doc.toObject();
const tojson = doc.toJSON();Diff is against the current |
Fix #13079
Summary
At runtime,
idis a virtual that Mongoose adds unlessidoption is disabled or there is already anidproperty on the schema. With this PR, TypeScript types will also addidas a virtual toTVirtuals. This removes theid?: anybit in theDocumentclass, which is suboptimal developer experience.Examples