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.

Sign up for GitHub

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

Jump to bottom

fix(model.d): fix findAndCountAll.count type #13736

Merged
merged 6 commits into from Dec 17, 2021
Merged

fix(model.d): fix findAndCountAll.count type #13736

merged 6 commits into from Dec 17, 2021

Conversation

zorji
Copy link
Contributor

@zorji zorji commented Dec 3, 2021 •
edited

Pull Request Checklist

Please make sure to review and check all of these items:

  • Have you added new tests to prevent regressions?
  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description Of Change

findAndCountAll should only return count as an array when the group option is provided.

This means in that case, options is no longer optional because when options is not provided, it's never a group count.
I also rearrange findAndCountAll order to ensure the single count overload is applied first.

Todos

  • [ ]

@zorji
fix(model.d): fix findAndCountAll.count type
987d177 
`findAndCountAll` should only return `count` as an array when the `group` option is provided.

This means in that case, `options` is no longer optional because when `options` is not provided, it's never a group count.
I also rearrange `findAndCountAll` order to ensure the single count overload is applied first.
@zorji zorji marked this pull request as draft December 3, 2021 01:53
@sdepold
Copy link
Member

sdepold commented Dec 3, 2021

Do you plan to continue the work on this PR or why is it in draft state?

@zorji
Copy link
Contributor Author

zorji commented Dec 4, 2021

Hi @sdepold

Yes, I do want to work on in, probably over the next weekend. But if you are interested in it feel free to contribute. I think it's only the test cases are incomplete.

@zorji zorji marked this pull request as ready for review December 13, 2021 05:28
@zorji
Copy link
Contributor Author

zorji commented Dec 13, 2021

Do you plan to continue the work on this PR or why is it in draft state?

The fix is done, could you help review too?

@sdepold sdepold added the type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense. label Dec 14, 2021
jesse23
jesse23 requested changes Dec 14, 2021
View reviewed changes
Copy link
Contributor

@jesse23 jesse23 left a comment •
edited

Choose a reason for hiding this comment

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

Could u help to update the doc too if possible? Thx...

docs/manual/core-concepts/model-querying-finders.md

also is it posible to update the comments(JsDoc/TsDoc) for 2nd impl?

@zorji
Copy link
Contributor Author

zorji commented Dec 17, 2021

Could u help to update the doc too if possible? Thx...

Hi @jesse23 I was trying to update the docs and I found the documentation and type for Model.count() is very wrong. I think it's better to fix the docs in a separated PR because the findAndCountAll() is based on the implementation of count().

i.e. look at this implementation here

sequelize/lib/model.js

Line 2052 in 5657a34

count: Number(item.count)

It's returning Array<Record<string, any> & { count: number }> but the doc and type says it returns number[].

I'll prefer to keep this PR as it is and address that doc later.

@sdepold sdepold merged commit b7b472e into sequelize:main Dec 17, 2021
@sdepold
Copy link
Member

sdepold commented Dec 17, 2021

Sounds good. Looking forward to another PR

@github-actions
Copy link
Contributor

🎉 This PR is included in version 6.12.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@zorji zorji deleted the patch-1 branch December 17, 2021 23:15
@zorji
Copy link
Contributor Author

zorji commented Dec 18, 2021

@sdepold @jesse23 started draft PR here to fix count type #13786

aliatsis pushed a commit to creditiq/sequelize that referenced this pull request Jun 2, 2022
@zorji @sdepold @aliatsis
fix(model.d): fix findAndCountAll.count type (sequelize#13736)
9b23fe0 
* fix(model.d): fix findAndCountAll.count type

`findAndCountAll` should only return `count` as an array when the `group` option is provided.

This means in that case, `options` is no longer optional because when `options` is not provided, it's never a group count.
I also rearrange `findAndCountAll` order to ensure the single count overload is applied first.

* refactor(model): add fix type for findAllAndCount

* refactor(model): tidy up type with SetRequired

Co-authored-by: Sascha Depold <sdepold@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jesse23 jesse23 jesse23 requested changes

@sdepold sdepold sdepold approved these changes

Assignees
No one assigned
Labels
released type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@zorji @sdepold @jesse23