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-4129): constrain watch type parameter to extend ChangeStream type parameter #3183

Merged
merged 2 commits into from
Mar 30, 2022

Conversation

eps1lon
Copy link
Contributor

@eps1lon eps1lon commented Mar 29, 2022

Description

What is changing?

Constrain watch type parameter to same constraint as ChangeStream

Is there new documentation needed for these changes?

Not

What is the motivation for this change?

Starting with microsoft/TypeScript#48366 we'll no longer be able to assign an unconstraint type parameter to a constraint one.

Constraining the type is required to pass DefinitelyTyped/DefinitelyTyped#59560

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@eps1lon eps1lon marked this pull request as ready for review March 29, 2022 20:25
@nbbeeken nbbeeken added the External Submission PR submitted from outside the team label Mar 30, 2022
@nbbeeken
Copy link
Contributor

Hi @eps1lon thanks for your help! Does only this change clear up the errors you are seeing? When I try compiling the driver with typescript@4.7.0-dev.20220330 I see an additional 33 errors mostly related to the same thing, just in other APIs.

Oh, or perhaps this is the only API usage you have so there are no other errors on your end. Can you confirm?

@eps1lon
Copy link
Contributor Author

eps1lon commented Mar 30, 2022

I was only seeing 3 errors:

Error: Errors in typescript@4.7 for external dependencies:
Error: ../meteor/node_modules/mongodb/mongodb.d.ts(1792,97): error TS2344: Type 'TLocal' does not satisfy the constraint 'Document'.
Error: ../meteor/node_modules/mongodb/mongodb.d.ts(2595,99): error TS2344: Type 'TSchema' does not satisfy the constraint 'Document'.
Error: ../meteor/node_modules/mongodb/mongodb.d.ts(4082,99): error TS2344: Type 'TSchema' does not satisfy the constraint 'Document'.

-- https://github.com/DefinitelyTyped/DefinitelyTyped/runs/5743860548?check_suite_focus=true#step:6:170

From what I can tell it's the same line that produces all 3 of those. But there may be more errors related to this issue.

@nbbeeken
Copy link
Contributor

Ah yea, our mongodb.d.ts file only has our public API and as I look closer I think the errors are all coming from internal types.

This change looks good to me but were you able to see a passing run with this change? Just confirming because I want to make sure this PR will unblock you and we can follow up with the errors I'm seeing.

@nbbeeken nbbeeken added the Team Review Needs review from team label Mar 30, 2022
dariakp
dariakp previously approved these changes Mar 30, 2022
@eps1lon
Copy link
Contributor Author

eps1lon commented Mar 30, 2022

This change looks good to me but were you able to see a passing run with this change?

Hard to judge since it's unclear what command produces the types. Just guessin from the naming that this fixes it.

@nbbeeken
Copy link
Contributor

I ran the tests locally, it looks like we need updates to the watch functions on Collection and MongoClient as well. I've updated those.

Just in case you missed it I'm still seeing an error from another package:

 ../styled-components/index.d.ts(119,68): error TS2344: Type 'P' does not satisfy the constraint '{ theme?: T | undefined; }'.

@nbbeeken nbbeeken requested a review from dariakp March 30, 2022 17:49
@eps1lon
Copy link
Contributor Author

eps1lon commented Mar 30, 2022

Just in case you missed it I'm still seeing an error from another package:

I can deal with that later. Thanks for pushing this further. The additional changes look good to me!

@nbbeeken nbbeeken changed the title fix(NODE-4129): Constrain watch type parameter to extend ChangeStream type parameter fix(NODE-4129): constrain watch type parameter to extend ChangeStream type parameter Mar 30, 2022
@nbbeeken nbbeeken merged commit 43ba9fc into mongodb:main Mar 30, 2022
@eps1lon eps1lon deleted the fix/ts-4.7-compat branch March 30, 2022 18:32
@eps1lon
Copy link
Contributor Author

eps1lon commented Mar 31, 2022

@nbbeeken Thanks for the quick assistance! Do know roughly when this'll be released?

@nbbeeken
Copy link
Contributor

nbbeeken commented Apr 4, 2022

@eps1lon Sorry about the delay, the fix is available now in v4.5.0!

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

Successfully merging this pull request may close these issues.

4 participants