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

New principle: A Promise represents completion or a value, not a callback (#342) #496

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

New principle: A Promise represents completion or a value, not a callback (#342) #496

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7905131
New principle: A Promise represents a value, not a callback (#342)
jan-ivar Jun 3, 2024
4eae825
Improve prose
jan-ivar Jun 4, 2024
7b0a008
This might be so that a return value can be examined...
jan-ivar Jun 11, 2024
2ac6cfe
Rephrase to discourage limitations on acting on a "settled result"
jan-ivar Jun 11, 2024
2825cbd
s/note/example
jan-ivar Jul 17, 2024
15258a4
new title and more work on examples
jan-ivar Jul 17, 2024
da09529
typo
jan-ivar Jul 17, 2024
c665547
s/time sensitive/timing sensitive/
jan-ivar Jul 17, 2024
a3d6a48
...resolution of the ...
jan-ivar Jul 26, 2024
8f15631
add sentence on when to use callbacks
jan-ivar Aug 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1877,6 +1877,37 @@ If the bytes in the buffer have a natural intepretation
as one of the other TypedArray types, provide that instead.
For example, if the bytes represent Float32 values, use a {{Float32Array}}.

<h3 id="allow-microtask-switches">A Promise represents a value, not a callback</h3>
jan-ivar marked this conversation as resolved.
Show resolved Hide resolved

Avoid requiring that something needs to happen synchronously within
a promise resolution or rejection callback.
Comment on lines +1882 to +1883
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this one can actually be "Never", because of the composition pitfall you're describing. Hm, except that you have "Never limit the viability..." down below, implying that you were trying for something more general here. How about:

Suggested change
Avoid requiring that something needs to happen synchronously within
a promise resolution or rejection callback.
When a Promise settles (is fulfilled or rejected),
that tells the developer that
certain facts have become true of the execution environment.
Developers tend to assume that these facts have "inertia":
that they will remain true until something acts on them.
When the platform automatically changes some of these facts,
that risks introducing bugs.
The platform must not change these facts between microtasks.
Always wait at least until a new task runs.

Copy link
Contributor

Choose a reason for hiding this comment

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

You seem to have hit on something useful that is at the core of this:

  • The state of the execution environment, as presented to the developer, cannot change except when executing a new task.

It's OK for the information presented to be a view of the state of things that might subsequently (or even immediately) be invalidated. If the API relates to something that is acting outside of the sandbox, like a camera or another application, then there is no way to be certain that information presented about that thing is correct. The best you can do is send it messages and see what happens. But things that comprise the environment are there to serve the developer's needs.

The whole Promise resolution thing is secondary to all of that. Maybe there is a different framing we could use that highlights that.

Copy link
Contributor Author

@jan-ivar jan-ivar Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the review and text!

The state of the execution environment, as presented to the developer, cannot change except when executing a new task.

I'm not sure we can say the environment is immutable. E.g. JS can call track.stop() and synchronously observe track.readyState changed from "live" to "ended". Only in-parallel changes seem problematic.

Also, apart from the busy-wait timer (which is more like ~1 second and a mitigation really), I don't think it's accurate to describe an overly strict and synchronous API contract as an in-parallel change to the environment.

The platform already seems to contain examples of synchronous API contracts, like preventDefault() that don't seem to violate § 5.2. Preserve run-to-completion semantics.

I think the anti-pattern is applying such synchronous API contracts to a promise callback.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, the environment can certainly change in response to the developer actively doing something. Thanks for pointing out "Preserve run-to-completion semantics". Maybe the bit about not changing things between microtasks should actually be part of that section. It could use some tweaking anyway because "while a JavaScript event loop is running" includes the time between tasks when we want the platform to change things.

But then what's left for this section to say? There is something interesting about the way that a Promise says "a new state has started", while a callback says "a state is present during this function invocation", but I'm not sure exactly how to express it. setFocusBehavior() is going to violate the rule regardless ... and maybe that's a sign that it should actually be a callback in the DisplayMediaStreamOptions, and not have special permission to run for a short time after the Promise resolves?

Copy link
Contributor Author

@jan-ivar jan-ivar Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

A singular underlying principle is encouraging, suggesting we're on the right track, or at least being consistent.

But practically speaking, might it still deserve two guidelines if the patterns to avoid are distinct enough?

Anti-pattern 1:

  • rationale: app must do something right away or not at all; a microtask window seems shorter than a task
  • symptom: breaks the async-functions-are-wrappable invariant

Anti-pattern 2:

  • rationale: mitigate busy-waiting exploits; a 1 second timer deadline won't bother well-behaved apps
  • symptom: breaks malicious or already broken apps as a secondary action-at-a-distance symptom

Reasons to treat them separately:

  • the first one seems more serious than the second
  • the second one seems in the long-running scripts mitigation category; exempt from guidelines?
  • setFocusBehavior() went from violating only the first to violating only the second; an improvement?

I'm concerned these distinctions might be lost if we merge/generalize the language too much.


Authors take for granted that an `async` function can be wrapped
in another that appends synchronous code at the end e.g. to inspect state
using try/catch. But the `await` queues a microtask:
jan-ivar marked this conversation as resolved.
Show resolved Hide resolved
<div class="example">
```js
async function fooWrapper() {
const foo = await platform.foo();
console.log("success");
return foo;
}

(async () => {
const foo = await fooWrapper();
foo.bar(); // on the same task, but not the same microtask that logged "success"
})();
```
</div>

To not interfere with this pattern, the lowest recommended restriction is "same task".
jan-ivar marked this conversation as resolved.
Show resolved Hide resolved

<div class="note">
jan-ivar marked this conversation as resolved.
Show resolved Hide resolved
One case where this came up was the [captureController.setFocusBehavior()](https://w3c.github.io/mediacapture-screen-share/#dom-capturecontroller-setfocusbehavior)
method, where timing concerns were solved by adding an timeout on top of
the same task requirement.
jan-ivar marked this conversation as resolved.
Show resolved Hide resolved
</div>

<h2 id="event-design">Event Design</h2>

<h3 id="one-time-events">Use promises for one time events</h3>
Expand Down