types: add overwriteMiddlewareResult and skipMiddlewareFunction to types

[vkarpov15]

Fix #14289

Summary

Looks like there's no way to use overwriteMiddlewareResult() and skipMiddlewareFunction() in TypeScript currently. This PR adds that, albeit with a couple of fake classes.

Examples

[hasezoey]

Looks OK, but why are the classes necessary?

[vkarpov15]

For the return signature of schema.pre() and schema.post() middleware functions. Pre middleware supports returning the result of skipMiddlewareFunction(), but returning any other value is a no-op. Similarly, post middleware supports returning the result of overwriteMiddlewareResult(), but returning a value of any other type is a no-op.

[vkarpov15]

@hasezoey what do you think?

[hasezoey]

i think i understand what the runtime does, but i am not quite sure how the types are supposed to work (especially the classes) maybe something other than classes would be more correct like interfaces? (because i dont see any classes being used to return, which could confuse some to not have the classes available at runtime)

[vkarpov15]

My understanding: if skipMiddlewareFunction() returns an interface like interface SkipMiddlewareFunction {}, that means TypeScript will treat return {} as equivalent to return mongoose.skipMiddlewareFunction(). That would be problematic.

Also, as best I can tell, since these classes aren't exported, there's no way for a user to accidentally try to instantiate them in TypeScript right?

[hasezoey]

Also, as best I can tell, since these classes aren't exported, there's no way for a user to accidentally try to instantiate them in TypeScript right?

they are exported (see export class), try the simple code and you wont get a typescript error, but get "full" types:

import * as mongoose from 'mongoose'; // mongoose@8.1.0 (pr #14328)
const cl = new mongoose.SkipWrappedFunction();

https://github.com/Automattic/mongoose/pull/14328/files#diff-093ad82a25aee498b11febf1cdcb6546e4d223ffcb49ed69cc275ac27ce0ccceR680-R684

when removing the export from export class:

Property 'SkipWrappedFunction' does not exist on type 'typeof import("mongoose")'.ts(2339)

Note that i only just discovered that they have export before it, so my messages before were going off-of a different though process

My understanding: if skipMiddlewareFunction() returns an interface like interface SkipMiddlewareFunction {}, that means TypeScript will treat return {} as equivalent to return mongoose.skipMiddlewareFunction(). That would be problematic.

i think so too, though i am not fully sure

[vkarpov15]

Would you be ok with it if we removed the export so it wasn't possible to instantiate these classes?

Another option: we add these classes to Kareem's TypeScript types. That would be the more technically correct way to do it, the current approach in this PR is a bit of a hack.

[hasezoey]

Would you be ok with it if we removed the export so it wasn't possible to instantiate these classes?

i think so, this would mean that the class wouldnt be able to be newed and would also ensure the functions would need to be called to obtain a "instance" of it (to return)

Another option: we add these classes to Kareem's TypeScript types. That would be the more technically correct way to do it, the current approach in this PR is a bit of a hack.

it would be more correct, but does kareem even have typescript types yet?

[vkarpov15]

Nope Kareem doesn't have types yet. This may be a forcing function for that.

[vkarpov15]

@hasezoey what do you think of this PR now with mongoosejs/kareem#37? I added Kareem types.

Right now ts-benchmark is failing, so I need to debug why the perf is degrading, but what do you think of the general idea?

[hasezoey]

i think it is now more correct (but would have worked before). the kareem PR has some work likely to-do before being merged, which also means this PR should not be merged before then.

Right now ts-benchmark is failing, so I need to debug why the perf is degrading, but what do you think of the general idea?

i dont know why the perf is degrading, but for the other error, i think it does not like that there is a class as a "top-level" element in the kareem types:

Error: node_modules/kareem/index.d.ts(26,1): error TS1046: Top-level declarations in .d.ts files must start with either a 'declare' or 'export' modifier.

maybe like i suggested in mongoosejs/kareem#37 (comment) the whole things needs to be wrapped in a declare module

[hasezoey]

will likely not re-review until the kareem dependency is a normal version again, or is there something else to test?

[vkarpov15]

@hasezoey not necessary to re-review, I'll merge this into 8.3.