mongoose icon indicating copy to clipboard operation
mongoose copied to clipboard

Why is `any` used as the type for `Document.id`?

Open juona opened this issue 2 years ago • 5 comments

Prerequisites

  • [X] I have written a descriptive issue title

Mongoose version

6.9.1

Node.js version

16

MongoDB version

5

Operating system

Linux

Operating system version (i.e. 20.04, 11.3, 10)

No response

Issue

The virtual id getter very clearly always returns a String:

function idGetter() {
  if (this._id != null) {
    return String(this._id);
  }

  return null;
}

However the type for the id field on Document objects is id?: any;.

Why not string? What am I not seeing?

juona avatar Feb 23 '23 13:02 juona

See https://github.com/Automattic/mongoose/pull/10248#pullrequestreview-660464370, #9773, etc.

We should be able to change this in Mongoose 7. But we need to keep id?: any in Mongoose 6 to support extends Document with non-string ids.

vkarpov15 avatar Feb 23 '23 15:02 vkarpov15

On further review, I think we still need to keep id: any for 7.0. That's because, in TypeScript, { id: string } & { id: number } resolves to { id: never } :man_facepalming:

image

With id?: any, everything works as expected:

image

We'll have to defer making this change until we manage to figure out how to use MergeType instead of & with Document<> in the HydratedDocument definition here: https://github.com/Automattic/mongoose/blob/05c30268277c7771cfa617f7e2ee359a237783a3/types/index.d.ts#L144-L146 . Right now, that causes a bunch of hard to debug test failures.

vkarpov15 avatar Feb 23 '23 15:02 vkarpov15

Gotcha! Thanks for the explanation. I knew there had to be a reason. Not a problem for me, feel free to close this as I was mostly just curious. : ]

juona avatar Feb 23 '23 17:02 juona

@vkarpov15 Was there a particular reason you aren't using a type union? There is no error when using a union instead:

type A = { id: string }
type B = A | { id: number }

const x: B = { id: 'bar' }
const x2: B = { id: 1 }

image

mjfwebb avatar Apr 24 '23 09:04 mjfwebb

@mjfwebb the problem is that Mongoose's HydratedDocument<> type uses &, not |.

vkarpov15 avatar Apr 25 '23 16:04 vkarpov15