Skip to content
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

[Types] da1aea152ef929bd33a54a462583a79265831d58 broke HydratedDocument with Record inputs #13094

Closed
2 tasks done
hasezoey opened this issue Feb 28, 2023 · 3 comments · Fixed by #13123
Closed
2 tasks done
Labels
typescript Types or Types-test related issue / Pull Request
Milestone

Comments

@hasezoey
Copy link
Collaborator

hasezoey commented Feb 28, 2023

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Mongoose version

7.0.0

Node.js version

does not apply

MongoDB server version

does not apply

Typescript version (if applicable)

4.9.5

Description

i have noticed that since da1aea1 (usage of MergeType) results in all thing using HydratedDocument with Record<string, any> (or unknown or never instead of any) results in all properties not being suggested, and when written out completely those types become whatever is set as the second parameter to Record

reverting to before da1aea1 fixes it

why does this matter?
because typescript-eslint has rule @typescript-eslint/ban-types, which is at level error by default when using preset @typescript-eslint/recommended, which suggests to instead use:

  • If you want a type meaning "any object", you probably want object instead.
  • If you want a type meaning "any value", you probably want unknown instead.
  • If you want a type meaning "empty object", you probably want Record<string, never> instead

Steps to Reproduce

Create a new model with either TInstanceMethods or TVirtuals set to Record<string, any> (or instead of any also unknown or never)
Or directly create a HydratedDocument like mongoose.HydratedDocument<DocType, Record<string, never>>

see Reproduction repository & branch

Expected Behavior

to allow usage of Record<string, never> at least, or Record<string, any> like before da1aea1


For anyone just looking for a workaround to have it working: use {}, but with disabling lint for the line // eslint-disable-next-line @typescript-eslint/ban-types

@hasezoey hasezoey added the typescript Types or Types-test related issue / Pull Request label Feb 28, 2023
@vkarpov15 vkarpov15 added this to the 7.0.1 milestone Feb 28, 2023
@hasezoey
Copy link
Collaborator Author

hasezoey commented Mar 2, 2023

i am also running into a weird issue where as long as there is a Require_id inside MergeType for HydratedDocument (MergeType<Require_id<DocType>, {}> ), typescript somehow misinterprets the the ref-types for typeguard functions in typegoose, but i cannot get it to reproduce in a testing project (or just mongoose types), also hover says the type is correct but then typescript decides it uses completely different types for the function itself - and that only happens for any document that does not use _id: ObjectId (like _id: string) and only removing MergeType (or the Omit behind it) solved it

@vkarpov15
Copy link
Collaborator

It looks like the issue is that MergeType<DocType, Record<string, never>> ends up setting never for every type. Why? TypeScript 🤷

image

We'll figure out some workaround. Probably just avoid merging record types somehow.

@hasezoey
Copy link
Collaborator Author

hasezoey commented Mar 3, 2023

for now i have been running with something like:

MergeType<
  Require_id<DocType>,
  TOverrides extends Record<string, never> ? {} :
    IfAny<TOverrides, {}>
  >

(not handling any or unknown, because those actually match everything and result in as if no special case was there)


Some more information about the previously mentioned MergeType, {}> issue

the above code change did not solve the typeguards issue i was also having, and are still unable to reproduce it anywhere else

ObjectId:
object_id_hover
objectid_fn_hover

String:
string_hover
string_fn_hover

where the definition is:

function isDocument<T, S extends RefType>(doc: Ref<T, S> | null | undefined): doc is DocumentType<T> {
  return doc instanceof mongoose.Model;
}

the weird thing is just, that without a MergeType<Require_id<>, {}> it just works - and correctly

this happened because i tried switching the custom type from typegoose DocumentType over to using HydratedDocument

typegoose's previous definition:

type DocumentType<T, QueryHelpers = BeAnObject> = mongoose.Document<unknown, QueryHelpers, T> &
  mongoose.Require_id<T> &
  IObjectWithTypegooseFunction;

typegoose's would-be new definition:

type DocumentType<T, QueryHelpers = BeAnObject> = mongoose.HydratedDocument<T, IObjectWithTypegooseFunction, QueryHelpers>;

which should be easy to switch over to HydratedDocument, which it also was before MergeType<Require_id<>, {}> was in use, but now it somehow breaks.

for now my resolution is to stay with the custom type

i guess this is just typescript being typescript

vkarpov15 added a commit that referenced this issue Mar 3, 2023
vkarpov15 added a commit that referenced this issue Mar 3, 2023
fix(types): handle `Record<string, never>` as value for HydratedDocument TOverrides parameter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typescript Types or Types-test related issue / Pull Request
Projects
None yet
2 participants