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

Model's type parameter must be defined #12212

Conversation

sandersn
Copy link
Contributor

@sandersn sandersn commented Aug 4, 2022

In Typescript 4.8 and mongodb since at least 4.8.1, mongodb.AnyBulkWriteOperation requires its type argument to be defined. It forbids null and undefined. Previous versions of Typescript didn't check this properly for unconstrained type parameters (lone type parameters like T). Typescript 4.8, which is in beta and will release in a week or two, does.

This PR adds a constraint to Model that requires its type parameter to be defined. It can no longer include null or undefined. In particular, that means Model<unknown> is no longer allowed; you have to write Model<{}> instead, which is the same type excluding null and undefined [1]. You can see this change in test/types/document.test.ts:179:

    expectType<Model<unknown>>(this.$model('Item1'));
// becomes
    expectType<Model<{}>>(this.$model('Item1'));

This change ripples out to other types as well, most notably the type parameter for Schema, which now also disallows null and undefined.

Alternative Fix

This is a pretty comprehensive PR. It should be possible to isolate the change to Model.bulkWrite by adding a conditional type:

mongodb.AnyBulkWriteOperation<T>
// would become
mongodb.AnyBulkWriteOperation<T extends {} ? T : any>

But this is unsafe since you can pass in writes that don't match Model's T, and because presumably AnyBulkWriteOperation really doesn't expect null or undefined and might crash at runtime.

I wouldn't recommend this smaller alternative except temporarily.

[1] unknown means "any value in Javascript". {} means "any value except null or undefined". object means "any non-primitive" (and does not allow null)

Recommendation

I'd recommend testing with typescript@beta during the Typescript beta period or, even better, typescript@next.

Fixes #12213

In Typescript 4.8 and mongodb since 4.8.1, mongodb.AnyBulkWriteOperation
requires its type argument to be defined. It forbids `null` and
`undefined`. Previous versions of
Typescript didn't check this properly for unconstrained type parameters
(lone type parameters like `T`). Typescript 4.8, which is in beta and
will release in a week or two, does.

This PR adds a constraint to Model that requires its type parameter to
be defined. It can no longer include `null` or `undefined`. In
particular, that means `Model<unknown>` is no longer allowed; you have
to write `Model<{}>` instead, which is the same type excluded `null` and
`undefined`. You can see this change in test/types/document.test.ts:179.

This change ripples out to other types as well, most notably the type
parameter for `Schema`, which now also disallows `null` and `undefined`.

This is a pretty comprehensive PR. It should be possible to isolate the
change to `Model.bulkWrite` by adding a conditional type:

```ts
mongodb.AnyBulkWriteOperation<T>
// would become
mongodb.AnyBulkWriteOperation<T extends {} ? T : any>
```

But this is unsafe since you can pass in writes that don't match Model's
`T`, and because presumably AnyBulkWriteOperation really doesn't expect
`null` or `undefined` and might crash at runtime.

I wouldn't recommend this smaller alternative except temporarily.
sandersn added a commit to sandersn/mongoose that referenced this pull request Aug 5, 2022
Smaller alternative fix to Automattic#12212.
Also fixes Automattic#12213, but unsafely when the model's type parameter allows
null or undefined.
@sandersn
Copy link
Contributor Author

sandersn commented Aug 5, 2022

I'm stuck trying to figure out how the type tests break, so I created #12221 for the small temporary fix, which will unblock people using Typescript 4.8 with mongoose.

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sandersn sorry for the late reply. Why is this change necessary? It looks like Mongoose's types tests work fine with typescript@4.8.1-rc. Can you come up with a script that fails to compile on 4.8.1-rc1 but should succeed?

@sandersn
Copy link
Contributor Author

I believe that the repro steps in #12213 still fail if you revert #12221 .

The problem is that #12221 makes typing less safe when the type parameter includes undefined. I believe it's not supposed to, but it would be better if all of the types correctly forbid undefined as with this PR. Unfortunately, I never got it to pass tests and ran out of time to work on it. You can close it if you don't want to pass it on to somebody else.

@vkarpov15
Copy link
Collaborator

I'll close for now. This doesn't seem like a huge problem, but I'm open to opposing viewpoints on that.

@vkarpov15 vkarpov15 closed this Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compile error with Typescript 4.8 beta on type from mongodb@4.8.1
2 participants