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: Filter query root operators support for partials #3136

Closed

Conversation

avaly
Copy link
Contributor

@avaly avaly commented Feb 8, 2022

Description

A change was introduced in #3102 in v4.3.1 regarding the new Filter type, which throws new errors for filter objects with root operators created dynamically.

What is changing?

I moved the RootFilterOperators<WithId<TSchema>> outside of the union inside the Filter<TSchema>.

This fixes the use case below, but it does break 3 lines in the find recursive test. I would love some help from @baileympearson, since you were the one to work on the recursive support.

Is there new documentation needed for these changes?

No.

What is the motivation for this change?

// this works in v4.3.1
const fooFilterBuiltNow: Filter<Foo> = {
  a: 'foo',
  $or: [{ a: 'bar' }]
};

const fooFilterBuiltLater: Filter<Foo> = {};
fooFilterBuiltLater.a = 'foo';
// this throws a TS error in v4.3.1
fooFilterBuiltLater.$or = [{ a: 'bar' }];

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

@baileympearson
Copy link
Contributor

baileympearson commented Feb 8, 2022

@avaly Is this a pressing change? There is a simple workaround with the existing types - use the RootFilterOperators type directly and spread them together as needed:

const operators : RootFilterOperators<Foo> = {
  $or: [{ a: 'bar' }]
}

// should be fine on 4.3.1 of the Node driver
const fooFilterBuiltLater: Filter<Foo> = {
  a: 'foo',
  ...operators
};

We discussed this issue last week and came to the consensus that while this is something we'd like to improve, we'd like to take the time to step back, properly evaluate the Filter type and come up with a design that should have fewer issues (and might even improve recursive type support). We have work set aside to address the Filter type but since the type isn't truly broken and there is a workaround, this is work we decided to hold off on for now.

@avaly
Copy link
Contributor Author

avaly commented Feb 8, 2022

@baileympearson Understood. It's not pressing, I just stumbled upon this today while trying to upgrade our codebase and hit this error a lot of times (due to how our code is structured).

@avaly avaly closed this Feb 8, 2022
@avaly
Copy link
Contributor Author

avaly commented May 30, 2022

@baileympearson Any update on this one?

We're running into this issue when using computed properties in a filter query:

await MetadataItem.updateMany({
  _id: { $nin: ids },
 [dynamicKey]: { $gt: 0 }
}, update);

TypeScript is complaining that Types of property '_id' are incompatible. because the filter is only tested against the Partial<TSchema> of the union type in Filter<TSchema>, most likely due to the dynamic key.

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.

2 participants