-
-
Notifications
You must be signed in to change notification settings - Fork 870
Conversation
Thanks @mitchlloyd! This looks great to me. @mixonic could you take a quick look over since I am not super familiar with this feature? We'll hold off on merging this until we're closer to the 2.3 release so that we can re-re-release 2.2 with fixes before 2.3 drops. |
@@ -191,11 +191,17 @@ export default Ember.Component.extend({ | |||
|
|||
## Factory Instance Lookups | |||
|
|||
The vast majority of Ember registrations and lookups are performed implicitly. | |||
You rarely need to write code to look up objects in an Ember application because |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"look up objects" seems both vague and like insider baseball here to me. Look up code from where? When might I be looking objects? I can imagine a developer new to Ember reading this and being confused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had written "look up dependencies" at one point, but that seemed pretty abstract. Also import
ing modules is "looking up dependencies". Maybe "You rarely need to use registered factories directly in an Ember Application"?
Any suggestions here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like
"In some cases, fetching an instantiated factory from the running application can be useful. The lookup
method on Ember.ApplicationInstance
objects provides this functionality. It takes an argument of a factory name, and returns an appropriate object. The application instance object is passed to Ember's instance initializer hooks, and can additionally be referenced as the "owner" of any object that was itself instantiated by the application instance object."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the concept of "you rarely need to do this" and "in some cases you might do this" for now. I'd rather just tell people how to do it and not talk about how often they should look to this technique unless we have some clear guidelines.
Reading again I feel that "look up objects" is less insider than the other phrases I'm reading for this in the docs. However, I do want to be as consistent as possible with the rest of the documentation so I've changed that. I went with @mixonic's "fetching an instantiated factory".
### Getting an Application Instance from a Factory Instance | ||
|
||
[`Ember.getOwner`][4] will retrieve the application instance from an Ember | ||
Object that was instantiated from a registered factory. This means that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something like
"Factory instances in Ember are "owned" by the application instance object that created them. The Ember.getOwner
function will retrieve the application instance object relevant to a factory instance, which may be useful for fetching other instances at runtime."
At the least "Ember Object" should probably not capitalize "Object" as it may imply something special about Ember.Object
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worked "owns" and "owner" into the added documentation. There is a description about the meaning of "owner" at the top of this documentation so I didn't want to repeat this too many times.
Seems pretty good to me, requires a rebase @mitchlloyd and lets do it |
5e91372
to
1633105
Compare
1633105
to
7cd8a36
Compare
Add documentation for `getOwner`
Thank you, @mitchlloyd! |
Looks like I shouldn't have merged this just yet. Sorry about that. |
I missed #1029 (comment) when reviewing so I am going to revert for now. So sorry about the confusion and extra work y'all. |
Revert "Add documentation for `getOwner`" Reverts #1029
:) no worries! |
Re-add getOwner documentation Re-adding documentation that was reverted. Original PR #1029
This is a PR to address #982.