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

Address concerns with injected properties feature #9760

Merged
merged 4 commits into from Dec 10, 2014

Conversation

slindberg
Copy link
Contributor

This addresses issues raised in the comments of #5162.

I haven't addressed the top two concerns yet because I'm still uncertain of how the container should be treated within Ember. I'm also not 100% sure about the onLookup API, e.g. whether it should be a hook or a callback mechanism.

if (Ember.FEATURES.isEnabled('ember-metal-injected-properties')) {
addOnLookupHandler();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rwjblue does this look right to you to avoid defeatureify nesting problems? There's a Ember.assert inside a Ember.runInDebug inside a Ember.FEATURES.isEnabled...

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me. Sorry for the odd hoops to jump through here, but I absolutely appreciate the effort.

@slindberg
Copy link
Contributor Author

The more I think about how the container validation currently works, the less I like it. I would love to implement a callback mechanism on the container like @stefanpenner suggested, but I can't see how that would work, mainly because Ember Object classes live in a world that is unaware of the container. The first time class code is aware of the container at all is on first instantiation by guessing at its own container property – ew.

Given that I can't come up with a better solution at this time, I've prepended an underscore to the two factory hooks that this feature has added to the container so they are officially private: _lazyInjections and _onLookup.

/cc @tomdale @stefanpenner @rwjblue

rwjblue added a commit that referenced this pull request Dec 10, 2014
Address concerns with injected properties feature
@rwjblue rwjblue merged commit ad4be79 into emberjs:master Dec 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants