Skip to content

Allow null as transactions errors value #460

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stelar7
Copy link
Contributor

@stelar7 stelar7 commented Jun 11, 2025

Some work towards fixing the issues mentioned in #433

I feel like introducing a separate flag/field for this would be clearer.
Or maybe passing an AbortError DOMException in IDBTransaction::abort() (But maybe this breaks something 🤷 )

The following tasks have been completed:

  • Confirmed there are no ReSpec/BikeShed errors or warnings.
  • Modified Web platform tests (link to pull request)

Implementation commitment:


Preview | Diff

Copy link
Collaborator

@SteveBeckerMSFT SteveBeckerMSFT 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 the fix! I left one question and potential update for us to discuss.

@@ -5144,8 +5148,7 @@ To <dfn>abort a transaction</dfn> with the |transaction| to abort, and |error|,

1. Set |transaction|'s [=transaction/state=] to [=transaction/finished=].

1. If |error| is not null, set |transaction|'s
[=transaction/error=] to |error|.
1. Set |transaction|'s [=transaction/error=] to |error|.
Copy link
Collaborator

@SteveBeckerMSFT SteveBeckerMSFT Jun 13, 2025

Choose a reason for hiding this comment

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

Do we need to include the condition that @inexorabletash suggested in his proposed minimal fix?

"If |error| us not set, set |transaction|'s [=transaction/error=] to |error|."

I think this might be needed if we call "abort a transaction" on a transaction that is already aborted.

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 this pull request may close these issues.

2 participants