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

[BUGFIX] Pass environment options forward to Engines #14025

Merged
merged 2 commits into from
Aug 5, 2016

Conversation

trentmwillis
Copy link
Member

Addressing #14015.

There are two commits here. The first simply adds two tests to verify the behavior of shouldRender: false in the visit API. The latter of those tests failed with an error due to attempting to render when it shouldn't (as described in the issue).

The second commit fixes the failing test by passing -environment:main from the parent into an EngineInstance. This also resulted in needing to update some tests to setup the registry in an ApplicationInstance that constructs the EngineInstance. I assume this will always happen in a real application, so I figured it made sense to do in the tests.

@@ -499,6 +499,9 @@ function commonSetupRegistry(registry) {

registry.injection('view:-outlet', 'namespace', 'application:main');

registry.injection('view', '_environment', '-environment:main');
registry.injection('route', '_environment', '-environment:main');
Copy link
Member

Choose a reason for hiding this comment

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

These injections should be done in the engine-instance (because the environment can change per .visit call).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Updated.

@stefanpenner
Copy link
Member

@rwjblue / @trentmwillis is this correct to target master?

@rwjblue
Copy link
Member

rwjblue commented Aug 4, 2016

The PR should target master with the commits prefixed with [BUGFIX beta] (since engines feature flag is enabled in beta).

Trent Willis added 2 commits August 4, 2016 11:18
This fixes a failing test introduced in the previous test where Engines
rendered even though shouldRender was set to false.
@trentmwillis
Copy link
Member Author

Updated the commit tags.

@rwjblue rwjblue merged commit eecc9f9 into emberjs:master Aug 5, 2016
@rwjblue
Copy link
Member

rwjblue commented Aug 5, 2016

Thanks @trentmwillis!

@rwjblue
Copy link
Member

rwjblue commented Aug 5, 2016

Pulled into beta branch, next build should fix for folks...

nathanhammond added a commit to nathanhammond/ember-engines that referenced this pull request Aug 8, 2016
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.

3 participants