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(types): update return type of Model.update #14155

Merged
merged 8 commits into from
Feb 24, 2022
Merged

fix(types): update return type of Model.update #14155

merged 8 commits into from
Feb 24, 2022

Conversation

guyellis
Copy link
Contributor

Pull Request Checklist

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

  • Did you update the typescript typings accordingly (if applicable)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description Of Change

As per the docs in the type I've updated the type.

A few lines above the change is a description where the update() method can return 2 types. I added the 2nd type.

src/model.d.ts Outdated Show resolved Hide resolved
@guyellis
Copy link
Contributor Author

Failing test looks like a random issue unrelated to this PR:

  1 failing

  1) [SQLITE] Transaction
       fails with SQLITE_BUSY when retry.match is changed:
     AssertionError: expected promise to be rejected with an error including 'SQLITE_BUSY: database is locked' but it was fulfilled with [ undefined, undefined ]
      at assertIfNotNegated (node_modules/chai-as-promised/lib/chai-as-promised.js:68:19)
      at /home/runner/work/sequelize/sequelize/node_modules/chai-as-promised/lib/chai-as-promised.js:183:17
      at async Context.<anonymous> (test/integration/transaction.test.js:739:9)

@WikiRik WikiRik requested a review from ephys February 24, 2022 08:35
Copy link
Member

@ephys ephys left a comment

Choose a reason for hiding this comment

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

Thank you for your PR!

I think we could handle this like Model.count: have two method signatures:

  • one with option.returning set to true that returns [affectedCount: number, affectedRows: M[]]
  • one without option.returning (or set to false) that returns [affectedCount: number]

src/model.d.ts Outdated Show resolved Hide resolved
@ephys ephys added the type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense. label Feb 24, 2022
@ephys ephys changed the title fix(typings): update returns union of types fix(typings): update return type of Model.update Feb 24, 2022
@ephys
Copy link
Member

ephys commented Feb 24, 2022

Also you're right, the sqlite issue is something that started happening randomly recently but unrelated to your PR

@WikiRik WikiRik changed the title fix(typings): update return type of Model.update fix(types): update return type of Model.update Feb 24, 2022
Co-authored-by: Zoé <zoe@ephys.dev>
@WikiRik WikiRik requested a review from ephys February 24, 2022 13:18
@guyellis
Copy link
Contributor Author

I think we could handle this like Model.count: have two method signatures:

  • one with option.returning set to true that returns [affectedCount: number, affectedRows: M[]]
  • one without option.returning (or set to false) that returns [affectedCount: number]

That was a great idea. Thank you! I've made the changes and pushed them. LMK what you think. Thanks.

src/model.d.ts Outdated Show resolved Hide resolved
guyellis and others added 2 commits February 24, 2022 07:43
Co-authored-by: Zoé <zoe@ephys.dev>
Copy link
Member

@ephys ephys left a comment

Choose a reason for hiding this comment

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

Thanks, I think we're good to go :)
Merging another PR first then I'll update & merge this one

@guyellis
Copy link
Contributor Author

Thanks, I think we're good to go :) Merging another PR first then I'll update & merge this one

Thank you for doing this! You are awesome! 🏆

@ephys ephys merged commit ce26f3c into sequelize:main Feb 24, 2022
ephys added a commit that referenced this pull request Feb 24, 2022
Co-authored-by: Zoé <zoe@ephys.dev>
Co-authored-by: Guy Ellis <guy.ellis@walmartlabs.com>
@ephys ephys mentioned this pull request Feb 24, 2022
ephys added a commit that referenced this pull request Feb 25, 2022
Co-authored-by: Zoé <zoe@ephys.dev>
Co-authored-by: Guy Ellis <guy.ellis@walmartlabs.com>
ephys added a commit that referenced this pull request Feb 25, 2022
Co-authored-by: Zoé <zoe@ephys.dev>
Co-authored-by: Guy Ellis <guy.ellis@walmartlabs.com>
@ruiaraujolink
Copy link

Hi, this PR create a problem, when I pass an array of attributes this happens
Screenshot 2022-03-04 at 13 58 40

@WikiRik
Copy link
Member

WikiRik commented Mar 4, 2022

Hi, this PR create a problem, when I pass an array of attributes this happens
Screenshot 2022-03-04 at 13 58 40

Hi! This should be fixed with #14184 right? That PR will be reviewed soon and then we will release a new version

@ruiaraujolink
Copy link

Hi, this PR create a problem, when I pass an array of attributes this happens
Screenshot 2022-03-04 at 13 58 40

Hi! This should be fixed with #14184 right? That PR will be reviewed soon and then we will release a new version

I think so, thanks.

@github-actions
Copy link
Contributor

🎉 This PR is included in version 7.0.0-alpha.11 🎉

The release is available on:

Your semantic-release bot 📦🚀

aliatsis pushed a commit to creditiq/sequelize that referenced this pull request Jun 2, 2022
Co-authored-by: Zoé <zoe@ephys.dev>
Co-authored-by: Guy Ellis <guy.ellis@walmartlabs.com>
vanthome pushed a commit to vanthome/sequelize that referenced this pull request Jun 12, 2022
Co-authored-by: Zoé <zoe@ephys.dev>
Co-authored-by: Guy Ellis <guy.ellis@walmartlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @v7 type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants