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

Generic types? #197

Closed
overflowz opened this issue May 17, 2020 · 22 comments · Fixed by #201
Closed

Generic types? #197

overflowz opened this issue May 17, 2020 · 22 comments · Fixed by #201
Assignees

Comments

@overflowz
Copy link

Hi there, first of all, thank you for the awesome work!

The issue -- is it possible to make Query to be a generic interface?
e.g. I am defining a generic repository that accepts a filter parameter that should be Query type of T.

Right now, Query (and it's properties) accept 'any' as a value.

Would be great to have:

export abstract class BaseRepository<T> {
  abstract find(item: Sift.Query<T>): Promise<T>;
  // ...
}

or when filtering arrays:

[{ name: 'foo' }, { name: 'bar' }]
  .filter(sift({ name: 'THIS_MUST_BE_STRING' }));

Thank you.

@crcn
Copy link
Owner

crcn commented Jun 4, 2020

This was a bit tricky, but it's finally done. This update has been added to v13.1.0, lmk if it works for you.

@overflowz
Copy link
Author

Thank you! I'll test as soon as I can and ping you here :)

@overflowz
Copy link
Author

@crcn unfortunately this passes TS check (even though it should not):

const objects = [{ string: 'hello', number: 1 }].filter(sift({
  string: 1234,
  number: 'world',
}));

I do get the variable names & types in autocomplete, but when I'm providing an invalid value (wrong type) then TS does not show me any errors (types are transformed into whatever types I provide there).

@crcn crcn reopened this Jun 4, 2020
@crcn
Copy link
Owner

crcn commented Jun 4, 2020

Just added a fix, let me know if it works!

@overflowz
Copy link
Author

overflowz commented Jun 4, 2020

almost there! I just noticed that $in operator is not available anymore ;p

Here's the example object I'm trying to test against:

const obj = {
  string: 'foo',
  number: 123,
  date: new Date(),
  boolean: true,
  arrayOfNumbers: [1, 2, 3],
  arrayOfStrings: ['a', 'b', 'c'],
  nestedArrayOfNumbers: [[1], [2], [3]],
  doubleNested: [[[1], [2]], [[1, 2]]],
  tripleNested: [[[[1]]]],
  arrayOfObjects: [{ a: 1 }, { a: 2 }],
  nestedArrayOfObjects: [[{ a: 1 }], [{ a: 2 }], [{ a: 3 }]],
  nested: {
    prop: true,
  },
  foo: {
    bar: {
      baz: 1,
    },
  },
};

query:

[obj].filter(sift({
  number: {
    $in: [123, 2, 3], // Type '{ $in: number[]; }' is not assignable to type 'number | alueQuery<number>   undefined'. Object literal may only specify known properties, and '$in' does not exist in type 'ValueQuery<number>'.
  },
}));

EDIT: also $nin is missing as well, haven't tested all of them, just a heads up :)

@overflowz
Copy link
Author

also! I think publishing these changes could break somebody's project in TS, because they might expect any (not using a strict version of TS) and wrong types could lead to errors in TS.

@crcn
Copy link
Owner

crcn commented Jun 4, 2020

I think publishing these changes could break somebody's project in TS, because they might expect any (not using a strict version of TS) and wrong types could lead to errors in TS.

Hmm, think this change should be a major bump? Figured that a minor bump would be enough since there aren't any behavioral changes.

Changes are up, let me know if they look good!

@overflowz
Copy link
Author

Everybody should have types matched anyway, if it's not then most likely it's a bug in their codebase (if I'm not wrong 😅) I'll check it out, thanks!

@overflowz
Copy link
Author

almost! the commented props in the code below works, however when I tried to use $in operator inside the nested props, it failed the check:

[obj].some(sift({
  // string: 'foo',
  // number: 123,
  // date: new Date(0),
  // boolean: true,
  // arrayOfNumbers: [1, 2, 3],
  // arrayOfStrings: ['a', 'b', 'c'],
  // nestedArrayOfNumbers: [[1], [2], [3]],
  // doubleNested: [[[1], [2]], [[1, 2]]],
  // tripleNested: [[[[1]]]],
  // arrayOfObjects: [{ a: 1 }, { a: 2 }],
  // nestedArrayOfObjects: [[{ a: 1 }], [{ a: 2 }], [{ a: 3 }]],
  // nested: {
  //   prop: true,
  // },
  foo: {
    bar: {
      baz: {
        $in: [1, 2, 3],
      },
    },
  },
})); // returns false

$in or $nin, they both return false. however if I specify strictly that it is equal to 1, then it works.

@crcn
Copy link
Owner

crcn commented Jun 4, 2020

Types around that should be fixed. You can't actually nest queries like that FYI

@overflowz
Copy link
Author

overflowz commented Jun 4, 2020

Oh, then it's all good, works for me :p (haven't used MongoDB queries for like ages).

EDIT: Thanks a lot!

@overflowz
Copy link
Author

Sorry, now this example is broken from the docs:

[
  { tags: ["books", "programming", "travel"] },
  { tags: ["travel", "cooking"] }
].filter(sift({ tags: { $all: ["books", "programming"] } }));

ts output:

Type '{ $all: string[]; }' is not assignable to type 'string[] | ArrayValueQuery<string> | undefined'.
  Object literal may only specify known properties, and '$all' does not exist in type 'string[] | ArrayValueQuery<string>'.

@crcn
Copy link
Owner

crcn commented Jun 4, 2020

K, try now!

@overflowz
Copy link
Author

two issues:

  1. RegExp does not work:
["craig", "john", "jake"].filter(sift(/^j/)); // Type 'RegExp' has no properties in common with type 'BasicValueQuery<string>'.
  1. $not does not seem to work (from the examples):
["craig", "tim", "jake"].filter(sift({ $not: { $in: ["craig", "tim"] } }));

ts output

Argument of type '{ $not: { $in: string[]; }; }' is not assignable to parameter of type 'BasicValueQuery<string>'.
  Object literal may only specify known properties, and '$not' does not exist in type 'BasicValueQuery<string>'.

@crcn
Copy link
Owner

crcn commented Jun 4, 2020

Getting types correct around this is so finicky, thanks for bearing with me 🙈. I added all examples to the types.ts test file, so hopefully, that covers everything else.

Changes have been published.

@overflowz
Copy link
Author

Thank you! Unfortunately, I'm out of time right now, but I'll test it tomorrow if you don't mind.

Cheers.

@crcn
Copy link
Owner

crcn commented Jun 4, 2020

No worries, thanks for taking the time to test!

@overflowz
Copy link
Author

Everything seems to work well, except one last part:

[{ name: "frank" }, { name: "joe" }].filter(
  sift({
    $where: function() {
      return this.name === "frank";
    }
  })
);

TS output:

Property 'name' does not exist on type 'Query<{ name: string; }>'.
  Property 'name' does not exist on type 'RegExp'.

happens when you type this.name because it cannot understand the context of this. this points to:

Query<{ name: string; }>

This is the final test I've made. I haven't really used much in the real app yet, just examples from the docs & minimal predictions.

@crcn
Copy link
Owner

crcn commented Jun 5, 2020

Odd, I'm unable to reproduce that particular bug. In any case, I added more type safety around $where ($where?: ((this: TValue) => boolean) | string), so I think the problem is fixed. Let me know if it works.

@overflowz
Copy link
Author

Yup, it got fixed! :) Thanks a lot!

@crcn
Copy link
Owner

crcn commented Jun 5, 2020

Awesome! Glad to hear that. Finally I can close this ticket. 👏

@crcn crcn closed this as completed Jun 5, 2020
@crcn
Copy link
Owner

crcn commented Jun 8, 2020

CC @stalniy, thought you might be interested in this.

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 a pull request may close this issue.

2 participants