Improve the run-to-completion principle. #536

jyasskin · 2024-12-06T13:42:46Z

"While an event loop is running" wasn't the right period to limit changes.
We have a list of exceptions to this principle.

This is meant to fix #456; @jakearchibald, can you check it? Thank you for the text!

I think this also supersedes #496. @jan-ivar, have I caught everything you meant to say there?

annevk · 2024-12-07T07:23:30Z

index.bs

+* Functions meant to help developers interrupt synchronous work,
+    as in the case of the proposed {{isInputPending()}}.


This feature is somewhat controversial because it violates run-to-completion.

Feels reasonable to drop this reference. The other examples are enough.

The TAG is on record supporting this exception in w3ctag/design-reviews#415 (comment) and w3ctag/design-reviews#475 (comment) (cc @dbaron for any historical context). I can break this into a separate PR if we need to discuss it more, but this function seems to be a case of putting authors ahead of technical purity. There might also be a better phrasing to explain what makes this exception acceptable?

If it's historical, maybe clarify this? MDN says this isInputPending is "superseded".

This guide is for new APIs, and as it reads, suggests "Functions meant to help developers interrupt synchronous work" is a legit reason. As general advice this might lead to new APIs that through blocking turn async calls into synchronous ones, which currently isn't possible AFAIK (a design invariant?)

Good catch. Apparently IdleDeadline.timeRemaining() also adjusts to help developers stop their idle work if other tasks appear, and it's in a WG and shipped in 2 engines. I'll switch to that example.

Suggested change

* Functions meant to help developers interrupt synchronous work,

as in the case of the proposed {{isInputPending()}}.

* Functions meant to help developers interrupt synchronous work,

as in the case of {{IdleDeadline/timeRemaining()|IdleDeadline.timeRemaining()}}.

I've filed w3c/requestidlecallback#104 because the spec doesn't call this out as clearly as I'd hope. I'm also not sure the name as clear about the unusual behavior as isInputPending was: do folks want to include advice that a better name would have been ideal, and if so, what name?

index.bs

jakearchibald · 2024-12-07T09:11:33Z

index.bs

+* Functions meant to help developers interrupt synchronous work,
+    as in the case of the proposed {{isInputPending()}}.


Feels reasonable to drop this reference. The other examples are enough.

jakearchibald · 2024-12-07T09:12:28Z

Generally LGTM!

jyasskin

Thanks for the reviews!

index.bs

jyasskin · 2024-12-07T19:28:12Z

index.bs

+* Functions meant to help developers interrupt synchronous work,
+    as in the case of the proposed {{isInputPending()}}.


The TAG is on record supporting this exception in w3ctag/design-reviews#415 (comment) and w3ctag/design-reviews#475 (comment) (cc @dbaron for any historical context). I can break this into a separate PR if we need to discuss it more, but this function seems to be a case of putting authors ahead of technical purity. There might also be a better phrasing to explain what makes this exception acceptable?

index.bs

dbaron

For what it's worth, I'm generally supportive of these changes. I think the core of this principle is about preventing surprising behavior and preventing broken code that results from that surprise. In that vein, the clearer it is that the functions have this sort of behavior, the better. (For example, it's reasonably clear that something like Date.now() might change as slow code executes.)

For things that do violate this, we want it to be pretty clear that code (conceptually, maybe not syntactically) like this is bad:

if (window.isPurple) {
  // (1)
} else {
  // (2)
}

// ...

if (window.isPurple) {
  // assume path (1) above executed
}

index.bs

martinthomson · 2024-12-13T02:50:52Z

index.bs

-Don't modify data accessed via JavaScript APIs
-while a JavaScript <a>event loop</a> is running.
-


I think that we need a positive statement for this principle.

Maybe

Propagate changes to state
that originate outside of JavaScript execution context
between tasks, not within them.

Yeah, or maybe by moving up the "a task should be queued" suggestion below...

If a change to state originates outside of the JavaScript execution context,
propagate that change to JavaScript between tasks,
for example by [[html#queuing-tasks|queuing a task]],
or as part of [=update the rendering=].

How's this? I also simplified the next paragraph, and focused it on async changes instead of claiming that https://html.spec.whatwg.org/multipage/webappapis.html#killing-scripts doesn't exist.

jan-ivar

Thanks, this looks like a great improvement to me!

I'd like to continue to discuss #496 separately for a little bit if that's OK, but that doesn't need to block this. It may be redundant, but I'd like to double-check there isn't a principle there worth describing specific to designing promise-returning APIs that come with synchronous requirements.

index.bs

jan-ivar · 2024-12-13T15:38:05Z

index.bs

+* {{HTMLImageElement/width|img.width}} to change as a result of loading image data from the network
+* Buttons of a {{Gamepad}} to change state
+* {{Element/scrollTop}} to change, even if scrolling can visually occur
+


My understanding is "changes" refers not only to readable properties, but any state changes that might trip up the developer's code execution, like the observable behavior of a method they invoke. Should we add an example of that? Maybe something like:

Suggested change

* A method acting differently depending on which [=microtask=] it is called on.

Yes, good point that we should specifically include changes in method behavior. I think the wording you've suggested implies that folks might be tempted to identify microtasks, which they generally aren't, so I'd like to call out a particular function. I had trouble finding a synchronous method that does actually change behavior depending on queued changes, and ones that return Promises can always return in a new task, so I think I have to use a counterfactual:

Suggested change

* A synchronous method to act differently depending on asynchronous state changes.

For example, if {{LockManager}} had synchronous methods,

their behavior would depend on concurrent calls in other windows.

A method acting differently depending on which [=microtask=] it is called on.

fwiw, a close example of this is requestAnimationFrame. It makes sense that it sometimes runs the callback before the next frame, and sometimes after, but it's annoying that you don't know which of these will happen. whatwg/html#10113

Hm, I can see a similar principle that requestAnimationFrame could violate, but all of the bits of the event loop transition on task boundaries, so it's not this principle. I didn't even mis-write this part of the example, since the rendering stages don't advance asynchronously. :)

Is this maybe a new principle for the HTML section that method behavior shouldn't depend on the event loop stage? Except that's not exactly what you're looking for in that issue since you're asking for a way to detect the stage rather than a method that behaves consistently. Could this be just "design requestAnimationFrame with the benefit of hindsight" rather than a generalizable principle?

Yeah, honestly, I think my example shows that:

A method acting differently depending on which [=microtask=] it is called on.

…isn't the right terminology.

I think requestAnimationFrame is ok, it's just missing that bit that tells you in advance if it'll run before or after the next frame.

Sounds good. I think that means https://w3ctag.github.io/design-principles/#js-rtc is basically correct, and there's nothing more for me to do on this PR. Let me know or file an issue if I have that wrong. :)

jan-ivar · 2024-12-13T15:54:47Z

index.bs

+* Functions meant to help developers interrupt synchronous work,
+    as in the case of the proposed {{isInputPending()}}.


If it's historical, maybe clarify this? MDN says this isInputPending is "superseded".

This guide is for new APIs, and as it reads, suggests "Functions meant to help developers interrupt synchronous work" is a legit reason. As general advice this might lead to new APIs that through blocking turn async calls into synchronous ones, which currently isn't possible AFAIK (a design invariant?)

* "While an event loop is running" wasn't the right period to limit changes. * We have a list of exceptions to this principle.

Co-authored-by: Martin Thomson <[email protected]>

… the middle of a statement.

Co-authored-by: Martin Thomson <[email protected]>

* It doesn't matter whether JS is a wrapper around code in another language. * JS doesn't always get to complete: users can kill it.

Co-authored-by: Jan-Ivar Bruaroey <[email protected]>

This can be reverted once WICG/shared-storage#212 is merged and crawled.

SHA: c63b666 Reason: push, by martinthomson Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

annevk reviewed Dec 7, 2024

View reviewed changes

jakearchibald reviewed Dec 7, 2024

View reviewed changes

jyasskin commented Dec 7, 2024

View reviewed changes

dbaron reviewed Dec 9, 2024

View reviewed changes

index.bs Outdated Show resolved Hide resolved

martinthomson reviewed Dec 9, 2024

View reviewed changes

index.bs Outdated Show resolved Hide resolved

martinthomson reviewed Dec 9, 2024

View reviewed changes

index.bs Outdated Show resolved Hide resolved

jyasskin commented Dec 9, 2024

View reviewed changes

index.bs Outdated Show resolved Hide resolved

jyasskin force-pushed the run-to-completion branch from f45db0c to 45b4ea4 Compare December 13, 2024 00:07

jyasskin added this to the 2024-12-16-week milestone Dec 13, 2024

martinthomson approved these changes Dec 13, 2024

View reviewed changes

jan-ivar reviewed Dec 13, 2024

View reviewed changes

jyasskin and others added 5 commits December 17, 2024 19:24

Improve the run-to-completion principle.

d892efa

* "While an event loop is running" wasn't the right period to limit changes. * We have a list of exceptions to this principle.

Warn against letting an attribute change in the middle of a statement.

4b0c905

Apply Martin's suggestions.

420d7ac

Co-authored-by: Martin Thomson <[email protected]>

Remove a redundant sentence of emphasis on not changing attributes in…

0875e30

… the middle of a statement.

Be more general than while true.

483f437

Co-authored-by: Martin Thomson <[email protected]>

jyasskin force-pushed the run-to-completion branch from da0caea to 483f437 Compare December 17, 2024 19:24

jyasskin and others added 3 commits December 17, 2024 20:43

Move a positive statement to the top of the principle.

088ec62

Remove irrelevant and incorrect claims.

4ea05e1

* It doesn't matter whether JS is a wrapper around code in another language. * JS doesn't always get to complete: users can kill it.

Apply changes resulting from Jan-Ivar's code review

7a896a6

Co-authored-by: Jan-Ivar Bruaroey <[email protected]>

jyasskin force-pushed the run-to-completion branch from ae181b3 to 7a896a6 Compare December 17, 2024 22:18

Identify which LockManager.

21262c9

This can be reverted once WICG/shared-storage#212 is merged and crawled.

martinthomson approved these changes Dec 17, 2024

View reviewed changes

martinthomson merged commit c63b666 into w3ctag:main Dec 17, 2024
1 check passed

github-actions bot added a commit that referenced this pull request Dec 17, 2024

Improve the run-to-completion principle. (#536)

3d599e1

SHA: c63b666 Reason: push, by martinthomson Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

jyasskin deleted the run-to-completion branch December 18, 2024 00:07

jyasskin mentioned this pull request Dec 18, 2024

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the run-to-completion principle. #536

Improve the run-to-completion principle. #536

jyasskin commented Dec 6, 2024 •

edited by pr-preview bot

Loading

annevk Dec 7, 2024

jakearchibald Dec 7, 2024

jyasskin Dec 7, 2024

jan-ivar Dec 13, 2024

jyasskin Dec 17, 2024 •

edited

Loading

jakearchibald Dec 7, 2024

jakearchibald commented Dec 7, 2024

jyasskin left a comment

jyasskin Dec 7, 2024

dbaron left a comment

martinthomson Dec 13, 2024

jyasskin Dec 13, 2024

jyasskin Dec 17, 2024

jan-ivar left a comment

jan-ivar Dec 13, 2024

jyasskin Dec 17, 2024 •

edited

Loading

jakearchibald Dec 18, 2024

jyasskin Dec 18, 2024

jakearchibald Dec 18, 2024

jyasskin Dec 18, 2024

jan-ivar Dec 13, 2024

		* Functions meant to help developers interrupt synchronous work,
		as in the case of the proposed {{isInputPending()}}.

		Don't modify data accessed via JavaScript APIs
		while a JavaScript <a>event loop</a> is running.


	* A method acting differently depending on which [=microtask=] it is called on.

+* A synchronous method to act differently depending on asynchronous state changes.
+    For example, if {{LockManager}} had synchronous methods,
+    their behavior would depend on concurrent calls in other windows.

Improve the run-to-completion principle. #536

Improve the run-to-completion principle. #536

Conversation

jyasskin commented Dec 6, 2024 • edited by pr-preview bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyasskin Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakearchibald commented Dec 7, 2024

jyasskin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-ivar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyasskin Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyasskin commented Dec 6, 2024 •

edited by pr-preview bot

Loading

jyasskin Dec 17, 2024 •

edited

Loading

jyasskin Dec 17, 2024 •

edited

Loading