Fix: Refactor LeanDocument type #11767

mohammad0-0ahmad · 2022-05-04T08:49:34Z

Related to #11761

taxilian · 2022-05-04T17:08:17Z

oh please please please don't do this. This does not fix leandocument -- it neuters it.

If you can give me some specific tests that you want to ensure work I will make sure they work without breaking everything that I spent days making sure worked. I promise there are reasons for all of the things that you just pulled out -- every single one of them. I would not have spent a week making sure they all worked otherwise.

mohammad0-0ahmad · 2022-05-04T17:13:09Z

Hello @taxilian !
Would you like to explain how does these changes "neuter" LeanDocument ?

mohammad0-0ahmad · 2022-05-04T17:26:39Z

@vkarpov15 @Uzlopak Please don't merge this PR until @taxilian explain why these changes are bad to have.

taxilian · 2022-05-04T17:30:49Z

The whole reason behind the "BaseDocumentType" abstraction is that you can't always get an accurate representation of the underlying type simply by subtracting the members that Document adds from the original. This makes it possible to get the original interface that was used to create the document instead of just wildly guessing and hoping that nobody did something unexpected like reusing something that document normally provides itself. It gives you a lot more accuracy and also makes types much more readable -- at least when following the normal patterns. If you read the getBaseDocumentType unit test which you completely slashed that should be clear -- there is no other way that you can actually get back the underlying type which you started with.

That said, this code isn't actually being used at all in the particular test that started the other thread, so I'm still working through your fix while I fix it in a way which doesn't undo all my hard work and make mongoose impossible to use with my code without providing my own version of the types (which I've been doing for years and would prefer not to do)

taxilian · 2022-05-04T17:41:35Z

So the goal of LeanDocument is to give you the document as it would be if you query it with lean() -- right? So here are the options:

Ideally we know the interface of the document that was placed inside -- when you grab it with lean then you should get that same document with no changes, unless some plugin or something has made those changes. The exceptions to that are that _id would be added if it wasn't overridden and __v would be added if it's not disabled. My personal argument is that at least __v should only be provided if the user wants to know about it, so they should be responsible for adding it to the interface. It has not ever been added by types before, so I don't think we should start now and set a bad pattern. I also disagree with adding _id, but it's not my call so I'll add it in -- not that hard anyway.
If we can't figure out the interface of the underlying document (e.g. if they extend Document instead of using generics) then the best we can do is say "Take the interface of the full mongoose document and subtract everything that is added when it is hydrated" -- which is what the original code did by omitting everything except _id, id, and __v. I think id shouldn't have been there since AFAIK by default it wouldn't be there on a lean document, but I didn't write that code so I left it as it was.

What this code does is instead says "Hey, I have an interface that I'll strip some array types out of and otherwise leave it alone". It basically doesn't try to pull out any of the things that LeanDocument was intended to pull out and removes 90% of the utility.

I have a fix for the original issue in #11761 mostly done which doesn't remove all the functionality, but I get the feeling there are additional things we should consider so if I can get any other tests to fix at the same time I'm happy to look at those as well.

If there is a better place we can chat about this I'm happy to do so.

taxilian · 2022-05-04T17:50:39Z

So honestly I wonder if the best / easiest way to address this is to just change the toObject return value like so:

toObject<T = DocType>(options?: ToObjectOptions): Require_id<LeanDocument<T>>;

We could make LeanDocument always add a _id but in some cases it may be used on a subdocument which may not have a _id (I always turn them off in my documents as they add bloat and are almost never needed for my stuff, but I don't know how common that is). It seems like it might be better to only assume they will be there on the root document, so the easiest way would be to just modify lean() and toObject to use Require_id and add the _id if it isn't there.

@mohammad0-0ahmad and/or @vkarpov15 thoughts?

Also, I don't understand why this would be new because the branch of types which I added isn't even being triggered for this test case..... but this is the test I'm using:

async function gh11761() {
  const thingSchema = new Schema<{ name: string }>({
    name: Schema.Types.String
  });

  const ThingModel = model('Thing', thingSchema);

  const { _id, ...thing1 } = (await ThingModel.create({ name: 'thing1' })).toObject();

  console.log({ _id, thing1 });
}

taxilian · 2022-05-04T17:53:08Z

AHH! I see why it happened; because I changed the return type of .toObject because using this there broke a bunch of stuff. lean() already has support correctly... I'll have a proposed PR soon.

taxilian · 2022-05-04T17:59:47Z

See #11769

mohammad0-0ahmad · 2022-05-04T19:11:23Z

With all you said @taxilian, I don't really get the reason why mongoose must have the mentioned types.
IMHO, I think your implementation increase the complexity of the code and make the TS server a bit slower without clear reason to do so.
As I said, keeping the deleted tests that you provided will still work in this PR by moving the unnecessary types to the test file, so It doesn't make sense "I think" to keep them.
Test this In my PR to double-check that.

// Helpers to simplify checks
type IfAny<IFTYPE, THENTYPE> = 0 extends (1 & IFTYPE) ? THENTYPE : IFTYPE;
type IfUnknown<IFTYPE, THENTYPE> = unknown extends IFTYPE ? THENTYPE : IFTYPE;

// tests for these two types are located in test/types/lean.test.ts
type DocTypeFromUnion<T> = T extends (Document<infer T1, infer T2, infer T3> & infer U) ?
  [U] extends [Document<T1, T2, T3> & infer U] ? IfUnknown<IfAny<U, false>, false> : false : false;

type DocTypeFromGeneric<T> = T extends Document<infer IdType, infer TQueryHelpers, infer DocType> ?
  IfUnknown<IfAny<DocType, false>, false> : false;

/**
 * Helper to choose the best option between two type helpers
 */
type _pickObject<T1, T2, Fallback> = T1 extends false ? T2 extends false ? Fallback : T2 : T1;

/**
 * There may be a better way to do this, but the goal is to return the DocType if it can be infered
 * and if not to return a type which is easily identified as "not valid" so we fall back to
 * "strip out known things added by extending Document"
 * There are three basic ways to mix in Document -- "Document & T", "Document<ObjId, mixins, T>",
 * and "T extends Document". In the last case there is no type without Document mixins, so we can only
 * strip things out. In the other two cases we can infer the type, so we should
 */
type BaseDocumentType<T> = _pickObject<DocTypeFromUnion<T>, DocTypeFromGeneric<T>, false>;
async function getBaseDocumentType(): Promise<void> {
  interface User {
    name: string;
    email: string;
    avatar?: string;
  }

  type UserDocUnion = User & Document<Types.ObjectId>;
  type UserDocGeneric = Document<Types.ObjectId, {}, User>;

  // DocTypeFromUnion should correctly infer the User type from our unioned type
  type fromUnion1 = DocTypeFromUnion<UserDocUnion>;
  expectType<User>({} as fromUnion1);
  // DocTypeFromUnion should give a "false" type if it isn't a unioned type
  type fromUnion2 = DocTypeFromUnion<UserDocGeneric>;
  expectType<false>({} as fromUnion2);
  // DocTypeFromUnion should give a "false" type of it's an any
  expectType<false>({} as DocTypeFromUnion<any>);

  // DocTypeFromGeneric should correctly infer the User type from our Generic constructed type
  type fromGeneric1 = DocTypeFromGeneric<UserDocGeneric>;
  expectType<User>({} as fromGeneric1);
  // DocTypeFromGeneric should give a "false" type if it's not a type made with Document<?, ?, DocType>
  type fromGeneric2 = DocTypeFromGeneric<UserDocUnion>;
  expectType<false>({} as fromGeneric2);
  // DocTypeFromGeneric should give a "false" type of it's an any
  expectType<false>({} as DocTypeFromGeneric<any>);

  type baseDocFromUnion = BaseDocumentType<UserDocUnion>;
  expectType<User>({} as baseDocFromUnion);

  type baseDocFromGeneric = BaseDocumentType<UserDocGeneric>;
  expectType<User>({} as baseDocFromGeneric);
}

async function getBaseDocumentTypeFromModel(): Promise<void> {
  interface User {
    name: string;
    email: string;
    avatar?: string;
  }
  const schema = new Schema<User>({});
  const Model = model('UserBaseDocTypeFromModel', schema);
  type UserDocType = InstanceType<typeof Model>;

  type baseFromUserDocType = BaseDocumentType<UserDocType>;

  expectType<User & { _id: Types.ObjectId }>({} as baseFromUserDocType);

  const a: UserDocType = {} as any;

  const b = a.toJSON();
}

mohammad0-0ahmad · 2022-05-04T19:11:43Z

Take a look at this as well:

mohammad0-0ahmad · 2022-05-04T19:12:51Z

I am leaving the final word for @vkarpov15 @Uzlopak to check both and merge the better one.

taxilian · 2022-05-04T19:23:53Z

@mohammad0-0ahmad given that the tests that you pulled out all specifically test a particular aspect of LeanDocument, yes if you pull that aspect out and then test only those things then of course the tests will pass -- that's the whole point of unit tests is to break down a problem into smaller pieces so you can test that unit of code.

Just because you can test a unit of code separately does not mean that the unit of code is not needed! It also doesn't change the fact that even completely ignoring those tests this PR fundamentally breaks LeanDocument by no longer removing the things added by Document!

Since it is apparently necessary I will make some tests to help demonstrate -- though if you actually look at the type output it should be pretty clear the difference, which is significant.

(also, I apologize I'm dealing with a minor catastrophe at work right now so taking time to do this is frustrating me more than it actually should, which is definitely not your fault -- trying to compensate, but having these removed would be absolutely disasterous for my usage of mongoose, which is why I spent most of a week writing these improvements and testing them in the first place. I don't have time to do this right now, but I also can't afford to have this discarded because the purpose wasn't understood.)

mohammad0-0ahmad · 2022-05-04T19:32:16Z

@taxilian as I said I've pulled out them because these tests needs the mentioned types.
Please provide which cases you are talking about that doesn't satisfy this PR when you have time for that.

taxilian · 2022-05-04T19:35:42Z

I do see that I misunderstood a few pieces of what is actually happening -- I'm still pretty sure that this will be a problem for my code, and it definitely results in less readable output in the IDE since it doesn't find its way back to the original types. I need to run this through all the tests with my code to see where it falls down which will take me a few hours -- but it's definitely fair for you to want me to check my assumptions, no matter how frustrated I am by other things or emotionally invested in my changes I may be =]

taxilian · 2022-05-04T19:47:10Z

Yeah, with this change there are now 37 errors in my codebase all due to LeanDocument related types. Unfortunately none of them are easy to track down -- it may take me a few hours to distill this into an explainable case / unit test.

Things like Argument of type 'LeanDocument<Applicant>' is not assignable to parameter of type 'LeanDocument<Document<unknown, any, Applicant> & Applicant & { _id: ObjectId; }>'. -- because previously those ended up broken down into the same type but now they are not

mohammad0-0ahmad · 2022-05-04T19:55:51Z

@taxilian Can you just provide code snippet to one of them ?

taxilian · 2022-05-04T20:12:47Z

The tricky bit is that I'm using an extra abstraction, so I actually define my schemas as a class with decorators -- it's a really nice system which lets me define things only once (instead of needing to define the types and schema separately) and also easily find the actual implementation of any bit of code using the browser. As nice as that is, I don't want to make compatibility with my abstraction the foundation of my argument for doing things a particular way if I can avoid it, so I want to try to reproduce this without my abstraction, but that requires a more contrived example, which is why it's going to take some time.

taxilian · 2022-05-04T20:20:06Z

https://github.com/gradecam/mongoose-decorators-ts/tree/feature/mongoose6 - this is the project, but I am pretty confident that I can demonstrate the issues in a more specific way.

vkarpov15 · 2022-05-04T20:57:16Z

I'm still reviewing this PR and #11769, I just wanted to make a quick comment about Require_id:

The reason why we have Require_id is because _id is optional when creating a new document by default, new Model({ foo: 'bar' }) is perfectly ok even though you don't specify _id. But Mongoose adds an _id by default. And, even if Mongoose didn't, the MongoDB server would add an _id. So while you technically can create a document without an _id in Mongoose, any document you load from MongoDB will have an _id unless you explicitly project it out. Hence we typically add Require_id for query results.

So adding Require_id to the result of toObject() isn't a terrible workaround. I wouldn't say it lines up exactly with how we expect Require_id to be used, but I'm happy to go that approach if it unblocks #11761.

mohammad0-0ahmad · 2022-05-04T21:01:22Z

I'm still reviewing this PR and #11769, I just wanted to make a quick comment about Require_id:

The reason why we have Require_id is because _id is optional when creating a new document by default, new Model({ foo: 'bar' }) is perfectly ok even though you don't specify _id. But Mongoose adds an _id by default. And, even if Mongoose didn't, the MongoDB server would add an _id. So while you technically can create a document without an _id in Mongoose, any document you load from MongoDB will have an _id unless you explicitly project it out. Hence we typically add Require_id for query results.

So adding Require_id to the result of toObject() isn't a terrible workaround. I wouldn't say it lines up exactly with how we expect Require_id to be used, but I'm happy to go that approach if it unblocks #11761.

In this PR, I think we don't need to add Require_id to the result of toObject(),
Please compare this PR with #11769 and pick one of them to merge.

taxilian · 2022-05-04T21:01:27Z

Okay, here is a simple test that shows one issue, though it's pretty contrived:

interface ThingType {
  name: string;
}
const thingSchema = new Schema<ThingType>({
    name: Schema.Types.String
});
const ThingModel = model('Thing', thingSchema);
type ThingDocument = HydratedDocument<ThingType>;
// or
type ThingyDocument = InstanceType<typeof ThingModel>;

function AcceptLeanDoc(val: LeanDocument<ThingDocument>) {
    // This should fail but it thinks the type is valid
    val.populate('');
}

Your type isn't actually constructing a lean type, it's starting with a type that is already lean -- which is great when you can do that, but there are cases where you can't or it's impractical. At best you have an extra type you have to pass around everywhere.

vkarpov15 · 2022-05-04T21:01:47Z

types/document.d.ts

@@ -132,7 +132,7 @@ declare module 'mongoose' {
    getChanges(): UpdateQuery<this>;

    /** The string version of this documents _id. */
-    id?: any;
+    id?: string;


This is a hard no. This would break anyone's code who uses extends Document with an id field that isn't a string.

This would break anyone's code who uses extends Document with an id field that isn't a string.
But then we have to update docs because it says it will be string.
Or users can omit id type before in type overriding case.

You can do new Schema({ id: Number }), that's perfectly valid in Mongoose. Mongoose just overrides the default id virtual.

taxilian · 2022-05-04T21:06:09Z

So I guess there exists the possibility that you don't see it as a needed thing to use LeanDocument on a HydratedDocument type and have it create the result of .lean(), but this would definitely be a breaking change since previously it did attempt to do that and now it doesn't

mohammad0-0ahmad · 2022-05-04T21:08:34Z

Okay, here is a simple test that shows one issue, though it's pretty contrived:
interface ThingType {
  name: string;
}
const thingSchema = new Schema<ThingType>({
    name: Schema.Types.String
});
const ThingModel = model('Thing', thingSchema);
type ThingDocument = HydratedDocument<ThingType>;
// or
type ThingyDocument = InstanceType<typeof ThingModel>;

function AcceptLeanDoc(val: LeanDocument<ThingDocument>) {
    // This should fail but it thinks the type is valid
    val.populate('');
}
Your type isn't actually constructing a lean type, it's starting with a type that is already lean -- which is great when you can do that, but there are cases where you can't or it's impractical. At best you have an extra type you have to pass around everywhere.

I think this will be so instead:

async function _11762() {
  interface ThingType {
    name: string;
  }
  const thingSchema = new Schema<ThingType>({
    name: Schema.Types.String
  });
  const ThingModel = model('Thing', thingSchema);

  function AcceptLeanDoc(val: LeanDocument<ThingType>) {
    // This should fail but it thinks the type is valid
    val.populate('');
  }
}

taxilian · 2022-05-04T21:11:01Z

Yes, you can do that -- it requires passing around an extra type, though, and it is definitely a breaking change. I don't know if anyone besides me is relying on it -- it would break all of my code, though I could probably work around it, it would just add a ton of complexity. It fundamentally changes how LeanDocument works and I'm not at all confident that we aren't using it somewhere else to do what it was originally intended to do -- convert a Document into a lean version of that document

mohammad0-0ahmad · 2022-05-04T21:17:13Z

I think Document third generic "DocType" is ThingType interface in your example, which is the default value for toObject Fn.
I might be wrong, but let's ask @vkarpov15 about that.
https://github.com/Automattic/mongoose/blob/master/types/document.d.ts#L13
https://github.com/Automattic/mongoose/blob/master/types/document.d.ts#L228

mohammad0-0ahmad · 2022-05-05T19:47:57Z

@vkarpov15 I am testing TS performance locally, and I got the following result,
But in GitHub it shows big difference in "check time" and "total time" values.

Do you have any idea why I got that? I am wondering if this workflow is returns consistent benchmark results.

taxilian · 2022-05-05T21:06:07Z

It's worth noting that even as relatively good as this benchmark is (and I found it super helpful when I was working on things) the results are really inconsistent across different systems with different loads. Just on my laptop I got significantly different numbers when I ran things e.g. 30 minutes later, even if it was the same thing. Presumably this is due to system load. Sometimes the difference would be negligible and sometimes significant -- I started looking at the complexity numbers more than the time because the time simply wasn't reliable enough across different runs.

mohammad0-0ahmad · 2022-05-05T21:09:10Z

@taxilian Would you like to review the new changes ?
I am working on options as well to make type more consistent.

taxilian · 2022-05-05T22:04:24Z

@taxilian Would you like to review the new changes ?
I am working on options as well to make type more consistent.

At first glance I'm completely baffled by a number of things, so I'm going to have to dig into it and I don't know if I can do it today. The first thing that I'm really confused about, though, is why you would move all the stuff you pulled out (the stuff that my code all needed, but you say isn't needed) into a unit test -- if it's not needed then it's not needed. If it is needed then it is needed; I can't imagine any situation where having it in a unit test would do anyone any good -- though if I can find some time to dig into it more deeply I may find something I'm missing.

taxilian · 2022-05-05T22:20:28Z

So I really don't understand what you are trying to fix with the updated changes.

You are fundamentally changing what LeanDocument does. I get that you don't think that it needs to do what it does -- that's a totally valid argument and discussion which can be had, since you obviously don't need it to do what it does. I however do need it to work the way that it has worked, though ideally it should work actually a little bit better than that (hence my changes). You can't just say "it doesn't need to do that because I've never needed to do that" and then make a breaking change in types as a bugfix... that's exactly what I accidentally did which started the whole discussion.

LeanDocument worked in a certain way... with this change it doesn't. It can certainly be argued that there are other ways to do those things, which seems to be your point, but that doesn't make it not a breaking change for anyone who is relying on it to do those things.

This PR fundamentally changes what LeanDocument does -- this isn't something that should be done in a patch release. What problem are you solving that isn't solved by the PR that he merged yesterday?

taxilian · 2022-05-05T22:30:59Z

Also, I did actually try the branch -- and all my stuff is broken exactly as expected because nested documents, document arrays, etc are all not correctly translated. It's possible that I could restructure my types in such a way that they would be, but that's the point -- this is a breaking change.

Uzlopak · 2022-05-05T23:03:30Z

Look guys. I think I have to make the unpopular decision and ask you to stop on this topic and end this "conflict".

LeanDocument got patched. For now we have fixed the issue. Maybe not the best way to do it, but it works now. Like the saying: "a solution is a temporary fix which becomes permanent". I assume you, @mohammad0-0ahmad are eager to simplify the typings, that is fine. But not now is not the time. We have no time pressure and we can be sure that with the next release that bug is not shipped.

I can guarantee you, that @vkarpov15 will not merge it as there is already no confidence that it wont break anything.

So, my decision is: I close this PR and propose another path to solve this:

@taxilian

I ask you to add more typings tests and cover all edge cases if necessary and provide a PR. Only test files for typings and if you find small bugs and you can fix them with little work, thats fine. But not more, no big typing changes regarding the LeanDocument topic.
Maybe we need to avoid testing the LeanDocument in expectType. This means: no "expectType<LeanDocument>(bla)" but "expectType<{ name: string }>(bla). So if we remove one day LeanDocument we wont need to rewrite the tests. So I assume that there is some work to do.

@mohammad0-0ahmad

When taxilian provided the typings tests, or says they are complete, we will review the PR. Like they say regarding TDD: "First measure and then cut." You are then free to provide a PR with refactored typings and as we already defined the tests, we would not need to touch the tests. So we can assure that there is no regression.

@taxilian
@mohammad0-0ahmad
I think you are both equally good devs. And that I propose this path to solve this issue and conflict,does not mean that you can not collaborate. On the contrary: I encourage you to work together. E.g. it could happen, that @taxilian finds some big bug while writing the typings tests, which needs alot of time to fix it with the current LeanDocument style. Then we could store the failing typings test on a topic branch in this repo and then you could work together to fix it by doing the big bugfix together.

I hope you stay motivated and dont take all this personal.

Best Regards
Aras aka uzlopak

taxilian · 2022-05-05T23:26:45Z

Very much agreed -- and please let me reiterate that I have no hard feelings towards anyone here, and I apologize for any "harsh tone" which I've probably used despite my best efforts -- it's mostly from fighting a losing battle with my ceph cluster all week.

@Uzlopak the PR which was merged by @vkarpov15 already has all of my things working, so I don't have any currently broken things that I need to fix, just things which this PR would have broken.

@mohammad0-0ahmad I actually (somewhat grudgingly ;-]) like the direction you're going with this, I was just concerned about trying to do it here in what I have perceived as being something intended to merge into 6.3.x. I would be happy to continue a discussion on a PR with longer term plans where we can try to hammer out some better type tests -- which I think we all can agree needs to happen. I really like what I've seen so far on the automatic type stuff, just trying to figure out how I can mesh it with how I'm doing things. please feel free to loop me in on anything you'd like reviewed for future stuff and we can go back and forth as needed in a less stressful way =]

Again, sorry my tone wasn't always great and for any disrespect that I conveyed, it was not intentional.

mohammad0-0ahmad · 2022-05-06T04:37:21Z

That is really disappointing that you are closing this PR again in that way.
I don't say that you have to merge it right now, but I would like to try to get better code and more reliable intellisense to get better UX and don't have backward breaking changes.

Firstly, I really need you @taxilian to understand that I am not targeting your code because it is yours, but I am just make things better and more straight forward,
Yesterday before merging that ANOTHER branch, THIS branch had some break changes like rely on only schema definition because I didn't know that mongoose allows extending Document in that way without passing the schema definition type as generic.
Therefore, THIS PR had this regression.
Just to clarify, you don't really need to be so confused that way about moving the mentioned TS helpers to test file.
Simply, the current tests are relying on these helpers, but mongoose in THIS PR don't require them at all to return lean document. But I would like to avoid touching current tests to avoid having regressions. I hope that you are understanding right now why I kept them in test file.

Secondly, please don't state something without be sure about it. You are saying that I am fundamentally changing what Lean Document does "which might possible", but please don't state that before reviewing the code. And it works, like it worked, "I believe". I am really know that I can't just say "it doesn't need to do that because I've never needed to do that". And I would never like to make a breaking change in types as a bug-fix or waste someone else efforts or time.
When you are argument that something, do some breaking change and don't cover all use cases, please mention that instead of just saying that a thing make fundamentally changes.
And I don't really agree with that should be in a patch release if It covers all current allowed use cases.
You say that all your stuff is broken, which stuff you means if all tests in test file that you wrote by self is passed, sorry, but I don't really like that way with augmenting I would prefer to show that case that is meant.

@Uzlopak with all my respect, but I don't take it as a conflict and as I said my only goal is better, simple code and more reliable intellisense even if @vkarpov15 would not like to merge it now, It can still as suggestion that can be discussed to get possibly better result in the feature.
Sorry If I am pressured someone here, but I just mentioned @taxilian to review the code when he has time even if this PR will not be merged It might be helpful to get new ideas to improve the current code without need to merge this one. So please don't consider that as conflict or competition to show who is better dev, just consider it subjective discussion and do not take it personally.

Talking about some new unit tests, I would like to encourage you to create tests to checks if __v is persistence in lean document and think about what should happening when supplying a generic to lean function, right now it doesn't add _id by doing so.

It might be also fantastic if we can get consistence lean document type when customizing toObject options argument.

At the end I do really apologize @taxilian @vkarpov15 @Uzlopak If you feel that I waste your time, or I waste your efforts @taxilian that you spent days for making sure it will work. And I will not be working on this one anymore for now, like you wish.

Best regards!

taxilian · 2022-05-06T07:28:24Z

I'll see if I can take some more time after I fix my other crisis to look over this, but I was not stating something without checking:

Previously with LeanDocument you could pass it a HydratedDocument type. With your change you cannot. I did review the code enough to be sure that was still the case, at least in every test that I did (though admittedly I only ran through things for about 10 minutes to see if I was missing anything, which was not thorough).

That is specifically the breaking change I was referring to -- you could pass in a HydratedDocument and get the base type out and in this PR you cannot.

Uzlopak · 2022-05-06T08:26:10Z

First of all, it was not about me feeling wasting my time or so. I was under the impression, that this gets a heated discussion. I wanted to moderate and avoid getting it more heated.

Because you both confirmed that this is not a heated discussion and claim to want to work further on this I assume there is no harm to reopen it.

Keep up the great work. :)

mohammad0-0ahmad · 2022-05-06T08:50:14Z

I'll see if I can take some more time after I fix my other crisis to look over this, but I was not stating something without checking:

Previously with LeanDocument you could pass it a HydratedDocument type. With your change you cannot. I did review the code enough to be sure that was still the case, at least in every test that I did (though admittedly I only ran through things for about 10 minutes to see if I was missing anything, which was not thorough).

That is specifically the breaking change I was referring to -- you could pass in a HydratedDocument and get the base type out and in this PR you cannot.

Alright, I've not noticed that, but that is great you could find that breaking change.
Send a code snippet for a test that shows this case you mentioned when you have time for that.

Uzlopak · 2022-05-30T08:47:36Z

Can you please fix the merge conflicts?
How is the state of this PR?

vkarpov15 · 2022-06-05T17:27:28Z

types/document.d.ts

+   * @summary Lean document base type.
+   */
+  interface LeanDocumentBaseType {
+    _id: Types.ObjectId;


This isn't sufficiently flexible. Not all lean documents have _id as an ObjectId. Maybe interface LeanDocumentBaseType<IdType = Types.ObjectId> { _id: IdType; } ?

vkarpov15 · 2022-06-05T17:28:26Z

types/document.d.ts

@@ -221,11 +253,11 @@ declare module 'mongoose' {
    set(value: any): this;

    /** The return value of this method is used in calls to JSON.stringify(doc). */
-    toJSON<T = LeanDocument<DocType>>(options?: ToObjectOptions & { flattenMaps?: true }): FlattenMaps<T>;
-    toJSON<T = LeanDocument<DocType>>(options: ToObjectOptions & { flattenMaps: false }): T;
+    toJSON<T = DocType>(options?: ToObjectOptions & { flattenMaps?: true }): FlattenMaps<LeanDocument<T>>;


I'd rather keep toJSON<T = LeanDocument<T>> to allow opting out in cases where LeanDocument doesn't quite work with how people wrote their code

vkarpov15 · 2022-06-05T17:29:38Z

test/types/lean.test.ts

+  type IfUnknown<IFTYPE, THENTYPE> = unknown extends IFTYPE ? THENTYPE : IFTYPE;
+
+// tests for these two types are located in test/types/lean.test.ts
+export type DocTypeFromUnion<T> = T extends (Document<infer T1, infer T2, infer T3> & infer U) ?


Are these helpers necessary? Don't seem used.

mohammad0-0ahmad · 2022-06-05T18:34:37Z

Can you please fix the merge conflicts? How is the state of this PR?

I was waiting for @taxilian response, but never mind, I will close this PR.

mohammad0-0ahmad changed the title ~~Refactor LeanDocument type~~ Fix: Refactor LeanDocument type May 4, 2022

mohammad0-0ahmad force-pushed the _11761 branch 2 times, most recently from 9d71615 to 716e93e Compare May 4, 2022 10:00

taxilian mentioned this pull request May 4, 2022

LeanDocument Type doesn't include _id or id #11761

Closed

vkarpov15 reviewed May 4, 2022

View reviewed changes

mohammad0-0ahmad force-pushed the _11761 branch 3 times, most recently from ad1827c to 1b20863 Compare May 5, 2022 19:28

mohammad0-0ahmad added 4 commits May 5, 2022 21:40

Refactor LeanDocument type & some related tests

6d43fe9

Delete unnecessary types and related tests.

7799c37

Fix regression mentiond by @vkarpov15.

51246fb

Refactor & write some jsDocs

a5d9bc1

mohammad0-0ahmad force-pushed the _11761 branch from 1b20863 to 9c48702 Compare May 5, 2022 19:40

Refactor after rebasing

6081b93

mohammad0-0ahmad force-pushed the _11761 branch from 9c48702 to 6081b93 Compare May 5, 2022 20:00

Uzlopak closed this May 5, 2022

temp commit.

92456b6

Uzlopak reopened this May 6, 2022

mohammad0-0ahmad marked this pull request as draft May 6, 2022 08:43

vkarpov15 reviewed Jun 5, 2022

View reviewed changes

mohammad0-0ahmad closed this Jun 5, 2022

Fix: Refactor LeanDocument type #11767

Fix: Refactor LeanDocument type #11767

Conversation

mohammad0-0ahmad commented May 4, 2022

taxilian commented May 4, 2022

mohammad0-0ahmad commented May 4, 2022 • edited Loading

mohammad0-0ahmad commented May 4, 2022

taxilian commented May 4, 2022

taxilian commented May 4, 2022

taxilian commented May 4, 2022

taxilian commented May 4, 2022

taxilian commented May 4, 2022

mohammad0-0ahmad commented May 4, 2022 • edited Loading

mohammad0-0ahmad commented May 4, 2022 • edited Loading

mohammad0-0ahmad commented May 4, 2022

taxilian commented May 4, 2022

mohammad0-0ahmad commented May 4, 2022

taxilian commented May 4, 2022

taxilian commented May 4, 2022 • edited Loading

mohammad0-0ahmad commented May 4, 2022

taxilian commented May 4, 2022

taxilian commented May 4, 2022

vkarpov15 commented May 4, 2022

mohammad0-0ahmad commented May 4, 2022

taxilian commented May 4, 2022

vkarpov15 May 4, 2022

Choose a reason for hiding this comment

mohammad0-0ahmad May 4, 2022

Choose a reason for hiding this comment

vkarpov15 May 4, 2022

Choose a reason for hiding this comment

taxilian commented May 4, 2022

mohammad0-0ahmad commented May 4, 2022

taxilian commented May 4, 2022 • edited Loading

mohammad0-0ahmad commented May 4, 2022 • edited Loading

mohammad0-0ahmad commented May 5, 2022 • edited Loading

taxilian commented May 5, 2022

mohammad0-0ahmad commented May 5, 2022 • edited Loading

taxilian commented May 5, 2022

taxilian commented May 5, 2022 • edited Loading

taxilian commented May 5, 2022

Uzlopak commented May 5, 2022 • edited Loading

taxilian commented May 5, 2022

mohammad0-0ahmad commented May 6, 2022 • edited Loading

taxilian commented May 6, 2022

Uzlopak commented May 6, 2022

mohammad0-0ahmad commented May 6, 2022

Uzlopak commented May 30, 2022

vkarpov15 Jun 5, 2022

Choose a reason for hiding this comment

vkarpov15 Jun 5, 2022

Choose a reason for hiding this comment

vkarpov15 Jun 5, 2022

Choose a reason for hiding this comment

mohammad0-0ahmad commented Jun 5, 2022

mohammad0-0ahmad commented May 4, 2022 •

edited

Loading

mohammad0-0ahmad commented May 4, 2022 •

edited

Loading

mohammad0-0ahmad commented May 4, 2022 •

edited

Loading

taxilian commented May 4, 2022 •

edited

Loading

taxilian commented May 4, 2022 •

edited

Loading

mohammad0-0ahmad commented May 4, 2022 •

edited

Loading

mohammad0-0ahmad commented May 5, 2022 •

edited

Loading

mohammad0-0ahmad commented May 5, 2022 •

edited

Loading

taxilian commented May 5, 2022 •

edited

Loading

Uzlopak commented May 5, 2022 •

edited

Loading

mohammad0-0ahmad commented May 6, 2022 •

edited

Loading