-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
add Addon.prototype.isEnabled for an addon to exclude itself from the project #2271
Conversation
@@ -187,7 +187,10 @@ Project.prototype.initializeAddons = function() { | |||
var addon = vertex.value; | |||
var AddonConstructor = Addon.lookup(addon); | |||
|
|||
project.addons.push(new AddonConstructor(project)); | |||
var initializedAddon = new AddonConstructor(project); | |||
if(!environment || !initializedAddon.isEnabled || initializedAddon.isEnabled(environment)) { |
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.
environment
being undefined here should not prevent isEnabled
from being called.
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.
The addon itself already knows the environment also now that I think about it (from this.app.env
since this.app
is set in the included
hook of the addon).
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 don't think it's been called at this point, get's called here I was thinking it was too, thus I agree with removing the argument if we can switch it up to be like that.
Seems like the addon's might need to be removed after being initialized in EmberApp.prototype._notifyAddonIncluded
. That way they can access the full app instance due to included being called. Seems a little dirty though
What do you think?
@rwjblue updated based on a few things. Mainly the ordering of hooks being called. Now it works how you'd expect. isEnabled is called prior to included but has the app set on the addon so you can use that for the enabled logic |
We need |
@rwjblue Since those are based on project.addons, would they be filtered out first here? or does it not always go through the EmberApp ? edit: I'm pretty sure I'm wrong, but I need to ask |
@rwjblue your 2 cents would be great. @jakecraige also tests are red |
I'm 👍 once the tests pass. |
ac0506d
to
e97a347
Compare
e97a347
to
5c452c2
Compare
5c452c2
to
182cef9
Compare
@rwjblue All fixed up now |
add Addon.prototype.isEnabled for an addon to exclude itself from the project
closes #2269
addons can exclude themselves by implementing an isEnabled hook where they can access
this.app
to do whatever checks they want to.I've also removed the requirement for settings this.app in the included hook. It always does it now.