Skip to content

fix(core): Add returning to DestroyOptions interface #17979

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
Open
conorjhughes wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(core): Add returning to DestroyOptions interface #17979

conorjhughes wants to merge 1 commit into from

Conversation

@conorjhughes
Copy link

@conorjhughes conorjhughes commented Sep 19, 2025

Pull Request Checklist

  • Have you added new tests to prevent regressions?
  • If a documentation update is necessary, have you opened a PR to the documentation repository?
  • 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?
  • Does the name of your PR follow our conventions?

Description of Changes

Adds a new property returning to the DestroyOptions interface.

List of Breaking Changes

None.

@conorjhughes conorjhughes requested a review from a team as a code owner September 19, 2025 11:44
@WikiRik
Copy link
Member

WikiRik commented Sep 29, 2025

I couldn't find any tests for the returning property, nor a mention of this feature. Are you sure this is supported?

@conorjhughes
Copy link
Author

conorjhughes commented Sep 30, 2025

@WikiRik It works when paranoid is enabled, but this is a side effect (a "happy accident") because destroy calls bulkUpdate in that case.

sequelize/packages/core/src/model.js

Line 2788 in 3a44a90

result = await this.queryInterface.bulkUpdate(

I can add a test for this if you're happy with this logic existing as-is?

I also need to update the return type of destroy to reflect returning.

sequelize/packages/core/src/model.d.ts

Line 2710 in 3a44a90

): Promise<number>;

@WikiRik
Copy link
Member

WikiRik commented Sep 30, 2025

I think in that case we should add an overload that's only active if paranoid is enabled and adds the BulkUpdateOptions. Unfortunately this part still has quite some work to do in terms of typing, so we'll see how much we can tackle in this PR.

It would be great if you could add tests that check the call to bulkUpdate when paranoid is enabled

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sdepold sdepold Awaiting requested review from sdepold sdepold is a code owner automatically assigned from sequelize/code-reviewers

@ephys ephys Awaiting requested review from ephys ephys is a code owner automatically assigned from sequelize/code-reviewers

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@conorjhughes @WikiRik