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

Allow app property to pass through addon proxy to addon instance #9936

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

raycohen
Copy link
Contributor

@raycohen raycohen commented Jun 8, 2022

When the EmberApp constructor runs, the presence of nested addon dependencies can result in the project.addons list containing a mixture of addon instances and addon proxy objects. For the addon proxy objects, the EmberApp setting their app property would prevent the addon instance from having it set, which goes against the expectation described in the models/addon.js app property comment:

this.project.addons.forEach((addon) => (addon.app = this));

/**
The host app instance.
**Note**: this property will only be present on addons that are a direct dependency
of the application itself, not of other addons. It is also not available in `init()`,
but will be set before `setupPreprocessorRegistry()` and `included()` are invoked.

Later on, when an addon attempts to read this.app it will be undefined if the addon was a Proxy by the time the EmberApp constructor method attempted to set it. This seems like a bug.

The comment explaining the proxy holding the app claims that ember-cli might set that app property, but I'm not sure what situation it is referring to. I didn't see any test coverage of this behavior. Simply removing this fixes the issue where addons in my project that expect to be able to read this.app in their index.js functions were erroring.

@raycohen raycohen marked this pull request as ready for review June 8, 2022 20:20
@kategengler kategengler requested a review from rwjblue June 10, 2022 18:11
@rwjblue
Copy link
Member

rwjblue commented Jun 10, 2022

@brendenpalmer - Would you mind reviewing?

@bertdeblock
Copy link
Contributor

Anyone who can verify this change? Would be nice to get this in if this actually fixes an issue.

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

3 participants