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

fixes issue #96: registry refactor to introduce registry entry objects #97

Merged
merged 10 commits into from
Dec 1, 2015

Conversation

@@ -171,7 +171,7 @@ When Reflect.Loader is called with no arguments, the following steps are taken:
1. If NewTarget is *undefined*, then throw a *TypeError* exception.
1. Let _O_ be OrdinaryCreateFromConstructor(NewTarget, "Reflect.Loader.prototype").
1. ReturnIfAbrupt(_O_).
1. Let _registry_ to a new Registry(_O_).
1. Let _registry_ to a new Registry().
Copy link
Contributor Author

Choose a reason for hiding this comment

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

registry doesn't need a backpointer to loader anymore.

Copy link

Choose a reason for hiding this comment

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

Let registry be

1. Let _entry_ be *this* value.
1. If Type(_entry_) is not Object, throw a new *TypeError*.
1. Let _stageValue_ be ToString(_stage_).
1. If _stageValue_ is an abrupt completion, return a promise rejected with a new *TypeError* exception.
Copy link

Choose a reason for hiding this comment

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

promise rejected with stageValue

Copy link
Member

Choose a reason for hiding this comment

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

Actually stageValue.[[value]]. Alternately I think you can just replace this whole step with RejectIfAbrupt(stageValue).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, this is very confusing because we use stageValue as a returned value and as a record of completion. I will use RejectIfAbrupt(stageValue).

* editorial changes to use ? instead of ReturnIfAbrupt.
* no more assertions to validate public methods, instead using If and throw.
* using verbose form to validate instances of Loader, Registry, Module and ModuleStatus.
* RejectIfAbrupt() when possible.
* all arguments in abstract operations are now validated.
* Removing unnecessary returns in promises that are not returned to the caller or stored.
1. Set _entry_.[[Dependencies]] to _deps_.
1. Set _entry_.[[Module]] to _instance_.
</emu-alg>

<emu-note>
[[Key]] wil identifies the dependency module status in the entry's corresponding loader.
Copy link

Choose a reason for hiding this comment

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

"wil identifies"?

* corrections on intrinsic object definitions
* CreateRegistry() abstract operation is the only way to create registry objects.
@caridy
Copy link
Contributor Author

caridy commented Nov 26, 2015

@caridy caridy mentioned this pull request Nov 30, 2015
@bmeck
Copy link

bmeck commented Dec 1, 2015

@caridy still looks like Registry has a Map of ModuleStatus as the inspection mechanism, which is still using Promises like candy for [[Result]].

@caridy
Copy link
Contributor Author

caridy commented Dec 1, 2015

@bmeck that's correct, but the inspection mechanism is now sync, you can essentially push new modules (resolved module entries) into the registry by creating new Reflect.Module.Status objects, and access them via loader, which is always going to be async.

@caridy
Copy link
Contributor Author

caridy commented Dec 1, 2015

fixes #98

@bmeck
Copy link

bmeck commented Dec 1, 2015

@caridy the registry cannot be inspected synchronously at all if none of the results can be inspected synchronously. it is fine if the user API uses promises for this, but the registry is still a no-go. Other comments around the repo look like they want to make subclassing the loader work with synchronous loading, but this is not possible if all registration results require turning the event loop.

@bmeck
Copy link

bmeck commented Dec 1, 2015

@caridy perhaps you can go through the inspection workflow in less text than the whole spec to show how to synchronously get the results?

@caridy
Copy link
Contributor Author

caridy commented Dec 1, 2015

@bmeck we can have a quick chat, and we can go over the details. just ping me!


The value of Reflect.Loader.prototype is an ordinary object.
The value of Loader.prototype is %LoaderPrototype%.
Copy link

Choose a reason for hiding this comment

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

The initial value of Loader.prototype is the intrinsic object %LoaderPrototype%.

@dherman
Copy link

dherman commented Dec 1, 2015

r+ with one fix for the issue where Registry !== Registry.prototype.constructor

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.

6 participants