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-4513): type for nested objects in query & update #3349

Merged
merged 5 commits into from
Aug 16, 2022

Conversation

jer-sen
Copy link
Contributor

@jer-sen jer-sen commented Aug 8, 2022

Description

Fix type for nested objects in query & update added in #3328

What is changing?

  • Support union types (... extends unknown ? ... : never)
  • Fail if path is wrong (never vs unknown)
  • Support array operators in the middle of a path (number replaced by number | $${'' | [${string}]} and NestedPathsOfType removed)
Is there new documentation needed for these changes?

No

What is the motivation for this change?

const _: MatchKeysAndValues<{ aaa: readonly { bbb: 'ccc' }[] }> = { 'aaa.$.bbb': 'ccc' }; // Error
const _: MatchKeysAndValues<{ aaa: { bbb: { ccc: 'ddd'[] }[] }> = { 'aaa.$.bbb.$[arrayFilter]': 'ddd' }; // Error

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

jer-sen added 2 commits August 8, 2022 17:41
- Support union types (... extends unknown ? ... : never)
- Fail if path is wrong (never vs unknown)
- Support array operators in the middle of a path (number replaced by number | `$${'' | `[${string}]`}` and NestedPathsOfType removed)
@nbbeeken nbbeeken added the External Submission PR submitted from outside the team label Aug 8, 2022
@nbbeeken
Copy link
Contributor

nbbeeken commented Aug 8, 2022

Hi @jer-sen, thanks so much for your help with our types! Would it be possible to add some tests to demonstrate the fix?
We use tsd to check our types, the files: test/types/community/collection/filterQuery.test-d.ts and test/types/community/collection/updateX.test-d.ts might be good spots to put some examples. You can run the type tests locally with npm run check:tsd

@jer-sen
Copy link
Contributor Author

jer-sen commented Aug 8, 2022

@nbbeeken sorry I will not have time for that, the fix works on my project, I took time to share it and give some simple repo but that's it. It will take to an already contributor less time to finalise what you are asking.

@nbbeeken nbbeeken changed the title Fix type for nested objects in query & update fix(NODE-4513): type for nested objects in query & update Aug 8, 2022
@nbbeeken
Copy link
Contributor

Just a heads up, there's a failure with a Map<string, number> type resolving one of the conditional types to never

Demonstrated by:
test/types/community/collection/filterQuery.test-d.ts:186:19

collectionT.find({ 'laps.foo': 123 });
// Type number is not assignable to type FilterOperators<never>

I'm looking into it on my end but let me know if you have any thoughts @jer-sen

@jer-sen
Copy link
Contributor Author

jer-sen commented Aug 11, 2022

@nbbeeken fixed!

Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

LGTM, just added some tests, thanks again for the help!

@nbbeeken nbbeeken added the Team Review Needs review from team label Aug 12, 2022
@jer-sen
Copy link
Contributor Author

jer-sen commented Aug 12, 2022

@nbbeeken you're welcome! Type checks are very useful. It could be further improved (arrayFilters, projections, first aggregate stages...)

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.

3 participants