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

related to issue #88: refactor to use a list of stages instead of internal slots for each state. #91

Merged
merged 4 commits into from
Oct 12, 2015

Conversation

caridy
Copy link
Contributor

@caridy caridy commented Oct 9, 2015

Changes:

  • state is now stagein the pipeline, which is more clear.
  • stages are stored in an ordered list, and each stage entry is removed as soon as the entry reaches a new stage.
  • renaming CommitInstantiate to ExtractDependencies.

Rendered HTML:

@@ -153,32 +153,13 @@ mean the same thing as:
1. Return the result of calling OrdinaryDefineOwnProperty(_obj_, _name_, _desc_).
</emu-alg>

<h4 id="get-state-value" aoid="GetStateValue">GetStateValue(state)</h4>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

stages are now in an ordered list, not need to track down the numeric values.

1. Return *undefined*.
1. Call SimpleDefine(_result_, "statePromise", _statePromise_).
1. If _entry_.[[State]] is "ready" then let _module_ be _entry_.[[Module]].
1. Call SimpleDefine(_result_, "stage", _stage_).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question: right now we are exposing stage and stagePromise as part of the shim for the registry entry. I think we can make this stage and value, which is just the thenable value for the entry's "ready" stage, or the current stage.

Copy link

Choose a reason for hiding this comment

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

Let's call them stage and result to match the semantics.

But we might want to consider exposing the full pipeline list. Something we should think about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

result it is then.

<emu-alg>
1. If Type(_entry_) is not Object, throw a *TypeError* exception.
1. Let _stages_ be _entry_.[[Pipeline]].
1. Return the first element of _stages_.
Copy link

Choose a reason for hiding this comment

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

add an assert: stages is non-empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it can't be empty. it is created by EnsureRegistered() or .install(), either way you can't never make it empty, you can only upgrade to an existent stage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a note about it.

@@ -584,7 +589,13 @@ Registry instances are initially created with the internal slots described in th
1. If _pair_ exists, then:
1. Let _entry_ be _pair_.[[value]].
1. Else:
1. Let _entry_ be a new registry entry record { [[Key]]: _key_, [[State]]: "fetch", [[Metadata]]: *undefined*, [[Fetch]]: *undefined*, [[Translate]]: *undefined*, [[Instantiate]]: *undefined*, [[Dependencies]]: *undefined*, [[Module]]: *undefined*, [[Error]]: *nothing* }.
1. Let _pipeline_ to be a new List.
Copy link

Choose a reason for hiding this comment

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

Let pipeline be

@dherman
Copy link

dherman commented Oct 9, 2015

Looks great. I have a couple tiny nits in the comments. Otherwise r+

caridy added a commit that referenced this pull request Oct 12, 2015
related to issue #88: refactor to use a list of stages instead of internal slots for each state.
@caridy caridy merged commit 2eff7f1 into whatwg:master Oct 12, 2015
@caridy caridy deleted the request-pipeline-state-bug-88 branch October 12, 2015 16:47
caridy added a commit that referenced this pull request Oct 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants