Skip to content
This repository has been archived by the owner on Aug 29, 2021. It is now read-only.

Editorial: Reinstate "evaluating-async" state alongside AsyncEvaluating -> AsyncEvaluation #179

Merged
merged 10 commits into from
May 19, 2021

Conversation

guybedford
Copy link
Collaborator

@guybedford guybedford commented May 14, 2021

This is in response to #171 by @kmiller68 where the usage of the ~evaluated~ state to indicate modules that are still currently async evaluating could be seen to be a confusing state definition. In addition, addressing a possible confusion that the [[AsyncEvaluating]] state has also been misunderstood before since it applies to both queued executions and running executions.

This is an editorial change which brings back the ~async-evaluating~ [[Status]] value which is set for any module either currently asynchronously executing, or waiting on a dependency that is currently asynchronously executing and also renaming [[AsyncEvaluating]] to [[QueuedEvaluation]] with a slight adjustment to completion handling where instead of treating execution completion as the transition of AsyncEvaluating to false as before, we then treat execution completion as the transition of the async-evaluating state to evaluated.

The previous issue with the ~async-evaluating~ status was that we still needed a separate field for AsyncEvaluating due to handling cycle transitions, as discussed in #114. The QueuedEvaluation field then still provides three purposes: iindicates which modules have async dependencies, it acts as the counter for async execution ordering and it handles within-cycle async queue attachment.

This PR can be seen as performing the following (non-normative) editorial replacement operations in order:

  • Replace all ~evaluated~ state checks with a check for ~evaluated~ or ~evaluating-async~.
  • Replace the ~evaluating~ -> ~evaluated~ transition for async executions with an ~evaluating~ -> ~evaluating-async~ transition.
  • Rename [[AsyncEvaluating]] to [[AsyncEvaluation]].
  • Append a transition of [[Status]] from evaluating-async -> evaluating at all sites where we previously set [[AsyncEvaluating]] to false.
  • Never transition [[AsyncEvaluation]] back to false, and instead entirely use the [[State]] == ~evaluated~ transitions for these checks.

This is editorial since the [[Status]] is currently entirely private to the module execution semantics, so this editorial change is more about understandability and ensuring the underlying state model is as clear as possible for future module system additions.

@guybedford guybedford requested a review from codehag May 14, 2021 14:28
Copy link
Collaborator

@codehag codehag left a comment

Choose a reason for hiding this comment

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

This looks good now.

@guybedford guybedford changed the title Editorial: Reinstate "async-evaluating" state alongside AsyncEvaluation -> QueuedEvaluation field Editorial: Reinstate "evaluating-async" state alongside AsyncEvaluation -> QueuedEvaluation field May 15, 2021
@guybedford guybedford requested a review from Ms2ger May 15, 2021 18:41
Copy link
Collaborator

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

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

I haven't done a super-thorough review, but the general idea seems to make sense. I'm happy to merge.

spec.html Outdated Show resolved Hide resolved
@guybedford guybedford changed the title Editorial: Reinstate "evaluating-async" state alongside AsyncEvaluation -> QueuedEvaluation field Editorial: Reinstate "evaluating-async" state alongside AsyncEvaluating -> AsyncEvaluation May 19, 2021
@codehag
Copy link
Collaborator

codehag commented May 19, 2021

we're doin' it. MERRRGE

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants