Skip to content

fix(NODE-4513): type for nested objects in query & update #3349

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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...)

# for free to join this conversation on GitHub. Already have an account? # 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