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

fix(NODE-3668): compile error with OptionalId on TS 4.5 beta #3004

Merged
merged 3 commits into from
Oct 18, 2021

Conversation

sandersn
Copy link
Contributor

Typescript 4.5 is better at checking certain complicated types than before. Previously, it missed an error in mongo_types.ts in the use of SetFields and OptionalId.

SetFields passes an unconstrained type parameter, TSchema, to OptionalId, which does have a constraint:

export type OptionalId<TSchema extends { _id?: any }> = ...

Because the type was so complex, Typescript missed this before 4.5.

Looking at the comments for OptionalId, I believe the intent is to add _id to any type, not just ones that already have an _id field. So I moved the constraint into a conditional type instead.

Fixes https://jira.mongodb.org/browse/NODE-3668?jql=project%20%3D%20NODE%20AND%20resolution%20%3D%20Unresolved%20AND%20text%20~%20%22typescript%22%20ORDER%20BY%20priority%20DESC%2C%20updated%20DESC

Description

What changed?

Types in mongo_types.ts

Typescript 4.5  is better at checking certain complicated types than
before. Previously, it missed an error in mongo_types.ts in the use of
SetFields and OptionalId.

SetFields passes an unconstrained type parameter, TSchema, to
OptionalId, which does have a constraint:

```ts
export type OptionalId<TSchema extends { _id?: any }> = ...
```

Because the type was so complex, Typescript missed this before 4.5.

Looking at the comments for OptionalId, I believe the intent is to add
`_id` to *any* type, not just ones that already have an `_id` field. So
I moved the constraint into a conditional type instead.
@nbbeeken nbbeeken changed the title Fix compile error on TS 4.5 beta fix(NODE-3668): compile error with OptionalId on TS 4.5 beta Oct 14, 2021
@nbbeeken
Copy link
Contributor

nbbeeken commented Oct 14, 2021

Hi @sandersn thanks so much for helping us out here!
I believe we're on the same page with the intent of OptionalId.

The ternary in OptionalId is meant to handle three cases:

  1. If there is an _id defined and it is not an ObjectId then it is a required field since the user must be creating keys, on the Node.js side.
  2. If there is an _id defined and it is an ObjectId then it can be optional since MongoDB will fill this in.
  3. If there is not an _id defined then it is also optional, but it is of type ObjectId.

I'm seeing some issues with our type tests that I'm going to investigate further soon, we're using tsd which I think is tied to TS version so it might not be actually testing these changes with the beta.

@nbbeeken nbbeeken self-requested a review October 14, 2021 22:11
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Oct 14, 2021
@sandersn
Copy link
Contributor Author

The only difference I can see between your prose and the code is that the check ObjectId extends TSchema['_id'] is backwards for (2) -- but there is a comment above justifying the backwardness, so I don't think it's worthwhile to change it.

@nbbeeken
Copy link
Contributor

I have a playground link here, while this may fix the crash upon import for SetFields it has broken the use case number 3. I'm trying to come up with a fix but if you see something here let me know. 🙂

@PaulEndri
Copy link
Contributor

I have a playground link here, while this may fix the crash upon import for SetFields it has broken the use case number 3. I'm trying to come up with a fix but if you see something here let me know. 🙂

Reading through that, is there a reason line 25 has WithId? If I'm reading it right, that type currently won't ever actually have _id as optional. Why would that not just be : TSchema ?

@sandersn
Copy link
Contributor Author

sandersn commented Oct 15, 2021

Playing with the link you sent, I'm pretty sure that the intention is to add an optional _id for case (3), but WithId adds a required one. This type eliminates WithId and makes the bottom else also optional. The code matches your prose cases (2), (1), (3) in that order.

export type OptionalId<TSchema> = TSchema extends { _id?: any }
  ? ObjectId extends TSchema['_id'] // a Schema with ObjectId _id type or "any" or "indexed type" provided
    ? EnhancedOmit<TSchema, '_id'> & { _id?: InferIdType<TSchema> } // a Schema provided but _id type is not ObjectId
    : EnhancedOmit<TSchema, '_id'> & { _id: InferIdType<TSchema> }
  : EnhancedOmit<TSchema, '_id'> & { _id?: InferIdType<TSchema> }; // TODO(NODE-3285): Improve type readability

I'm pretty sure this worked by mistake before when you pass an object without _id, because ObjectId extends TSchema['_id'] ? ... is true because the constraint says that _id: any, and ObjectId extends any because everything extends any.

Edit: for comparison, here is the pre-PR version:

export type OptionalId<TSchema> = TSchema extends { _id?: any }
  ? ObjectId extends TSchema['_id'] // a Schema with ObjectId _id type or "any" or "indexed type" provided
    ? EnhancedOmit<TSchema, '_id'> & { _id?: InferIdType<TSchema> } // a Schema provided but _id type is not ObjectId
    : WithId<TSchema>
  : WithId<TSchema>; // TODO(NODE-3285): Improve type readability

@sandersn
Copy link
Contributor Author

sandersn commented Oct 15, 2021

I pushed a commit to the PR that makes _id optional for case (3).

@nbbeeken
Copy link
Contributor

I was able to test locally with tsd using TS 4.4 and 4.5 and it passes 🚀
Thanks! I agree with you that it did work by mistake before, so I appreciate the improvement. I'll turn this over to the team for a look and we'll get this in soon.

Also thanks @PaulEndri for takin a look as well!

@nbbeeken nbbeeken requested a review from dariakp October 15, 2021 18:14
@nbbeeken nbbeeken added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Oct 15, 2021
@nbbeeken nbbeeken requested a review from durran October 15, 2021 18:15
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

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

LGTM, thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants