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 typeof Model errors throughout by using typeof Model generics #900

Merged
merged 9 commits into from
Feb 14, 2021
Merged

Fix typeof Model errors throughout by using typeof Model generics #900

merged 9 commits into from
Feb 14, 2021

Conversation

KapitanOczywisty
Copy link
Contributor

Update of #868 with fixed lint errors. #868 (comment)

cc: @divlo

@codecov
Copy link

codecov bot commented Feb 6, 2021

Codecov Report

Merging #900 (9fb9f44) into master (7c467d4) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #900   +/-   ##
=======================================
  Coverage   95.20%   95.20%           
=======================================
  Files         117      117           
  Lines        1127     1127           
  Branches      129      129           
=======================================
  Hits         1073     1073           
  Misses         22       22           
  Partials       32       32           
Impacted Files Coverage Δ
src/model/model/model.ts 100.00% <ø> (ø)
...ons/belongs-to-many/belongs-to-many-association.ts 92.30% <100.00%> (ø)
...rc/associations/belongs-to-many/belongs-to-many.ts 90.00% <100.00%> (ø)
.../associations/belongs-to/belongs-to-association.ts 100.00% <100.00%> (ø)
src/associations/belongs-to/belongs-to.ts 85.71% <100.00%> (ø)
...rc/associations/foreign-key/foreign-key-service.ts 88.88% <100.00%> (ø)
src/associations/foreign-key/foreign-key.ts 100.00% <100.00%> (ø)
src/associations/has/has-association.ts 100.00% <100.00%> (ø)
src/associations/has/has-many.ts 87.50% <100.00%> (ø)
src/associations/has/has-one.ts 87.50% <100.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c467d4...9fb9f44. Read the comment docs.

Copy link
Contributor

@theoludwig theoludwig left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! @KapitanOczywisty
That's great to have a stricter model defintion!

What if I don't want to provide <TCreationAttributes, TModelAttributes> is it still working ?
It should still works!

Also see my comment. 😄
We should avoid as much as possible any type, sometime we can't, we need to use any, we can't do something else.

@KapitanOczywisty
Copy link
Contributor Author

What if I don't want to provide <TCreationAttributes, TModelAttributes> is it still working ?

In my testing both ways work now, seems like @intellix did a great job. When you don't provide attributes you have effectively <{}, {}>.

Copy link
Member

@RobinBuschmann RobinBuschmann left a comment

Choose a reason for hiding this comment

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

Hey @KapitanOczywisty thanks for fixing this. Can you add some tests please?

@KapitanOczywisty
Copy link
Contributor Author

@RobinBuschmann I probably could add a couple of types-only tests. What do you want to see in these tests?

@RobinBuschmann
Copy link
Member

@RobinBuschmann I probably could add a couple of types-only tests. What do you want to see in these tests?

The tests should validate that it's now possible to use <TCreationAttributes, TModelAttributes>. Yeah, checking the types should be enough 👍

@KapitanOczywisty
Copy link
Contributor Author

Sooo, I see problems near everything what is touching through in BelongsToMany I have to rewrite this a bit.

And there are also problems with FooModel.findAll({include: BarModel}), but this is due to sequelize itself, so leave that for them to deal with?

@KapitanOczywisty
Copy link
Contributor Author

@RobinBuschmann Fixed BelongsToMany and some testing added.

@KapitanOczywisty
Copy link
Contributor Author

KapitanOczywisty commented Feb 7, 2021

For userland type-guard have to be a bit more finicky and I'm thinking about exporting this one to userland, where you need to access some static properties from Model. I'd like to hear some feedback on that.

Generally, type ModelType = Properties<typeof Model> also works fine as type-guard and is fine when you don't care about attributes (which is the case in example bellow).

type Properties<T> = {
  [P in keyof T]: T[P];
}
type ModelType<TModelAttributes = any, TCreationAttributes = TModelAttributes> = Properties<typeof Model> & (new () => Model<TModelAttributes, TCreationAttributes>);

// ---

export function SequelizeFilter(model: ModelType, filterable: string[], filter?: string): WhereOptions | undefined {
  if (!filter || filter === "") return;

  const fields = [];
  for (let prop of filterable) {
    // rawAttributes works here because of Properties<typeof Model>
    // otherwise there would be: Property 'rawAttributes' does not exist on type 'ModelType<any, any>'.
    if (model.rawAttributes?.[prop]) {
      fields.push({ [prop]: { [Op.substring]: filter } });
    }
  }

  return fields.length ? { [Op.or]: fields } : undefined;
}

Edit: There is type type Repository<M> = (new () => M) & NonAbstract<typeof Model> exported already.

Copy link
Member

@RobinBuschmann RobinBuschmann left a comment

Choose a reason for hiding this comment

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

Sorry for the late reply: Good work 👍

@intellix
Copy link
Contributor

intellix commented Feb 21, 2021

Awesome, thanks for moving this forward :)

It seems my initial issues weren't addressed though in regards to Includeables: #868 (comment)

Comment here: #835 (comment)

It looks like it was acknowledged in the comment above that it should be fixed within Sequelize itself

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants