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

Sketch out spec text for transaction durability option #301

Merged
merged 1 commit into from
Jan 22, 2020

Conversation

inexorabletash
Copy link
Member

@inexorabletash inexorabletash commented Sep 26, 2019

Closes #50

The following tasks have been completed:

  • Confirmed there are no ReSpec/BikeShed errors or warnings.
  • Modified Web platform tests (tentative tests)

Implementation commitment:


Preview | Diff

@pwnall
Copy link

pwnall commented Sep 26, 2019

I don't think we discussed whether we'd want options to be an alternative to mode. Worth figuring this out before we commit to the API shape described here?

@asutherland
Copy link
Collaborator

Excellent phrasing on the spec text. I don't think I personally have an opinion on the signature other than the overload stuff at https://heycam.github.io/webidl/#idl-overloading is complicated and the presence of optional arguments complicates things. (Specifically, in our attempt to eliminate Firefox's overload of open and use-counting we've run into things.) It seems reasonable to me to stick to not having any overloads if we think we can avoid future overloads, which would be an argument for the current pull request's approach. If we'll want overloads in the future, we maybe should consider all of this more strongly.

@inexorabletash
Copy link
Member Author

inexorabletash commented Sep 26, 2019

options to be an alternative to mode

As a third argument, it's a graceful fallback for older browsers - silently ignored. Which is great IMHO since it's just a hint. And it avoids an overload, which we're generally allergic to. Open to counter-arguments, though.

A few things the patch needs:

  • It needs to say "The durability hint is one of the following:"
  • It needs to explicitly say that the default for transaction objects is "default" (e.g. for versionchange transactions)
  • The text should not just be tucked in to the bottom of the transaction definition - it might make more sense a few paragraphs up.
  • Do we want to throw if a non-default option is set for readonly transactions? IMHO: no.

@pwnall
Copy link

pwnall commented Dec 6, 2019

Do we want to throw if a non-default option is set for readonly transactions? -- I agree with you.

It makes more sense to me that all transactions have a durability property that behaves in the same way. Since it's intended to be a hint, the hint will be ignored in read-only transactions.

@inexorabletash
Copy link
Member Author

Any objections to going forward with merging this?

@asutherland
Copy link
Collaborator

I tried to be cool in December and use a thumbs-up emoji, but I don't think that generated a notification, so to clarify, I'm good with merging this.

@inexorabletash
Copy link
Member Author

Hah! Thanks for remembering.

@inexorabletash inexorabletash merged commit 1a519e7 into master Jan 22, 2020
@inexorabletash inexorabletash deleted the durability branch January 22, 2020 20:30
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.

Define flush-to-disk guarantees and control
3 participants