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

Engine routes deactivated after destruction #14234

Closed
trentmwillis opened this issue Sep 8, 2016 · 7 comments
Closed

Engine routes deactivated after destruction #14234

trentmwillis opened this issue Sep 8, 2016 · 7 comments

Comments

@trentmwillis
Copy link
Member

trentmwillis commented Sep 8, 2016

In 2.8.0, there was a change introduced to the order of teardown when Engines are involved. The change in question seems to have fixed another issue, but causes routes in Engines to be destroyed prior to their deactivate hook being run (during the Router's reset).

This means that if you do something such as set a property on the Route during deactivate, then errors will propagate in Acceptance tests.

Unsure what the proper fix is here. Seems like maybe only the views from the Engines should be torn down prior to resetting the Router.

(Here's the prior implementation from the ember-engines addon.)

cc @asakusuma @krisselden

@rwjblue
Copy link
Member

rwjblue commented Sep 8, 2016

@trentmwillis - Can you work up a failing test (best harness ATM for this is here)?

@trentmwillis
Copy link
Member Author

@rwjblue reproduction added in #14237.

@trentmwillis
Copy link
Member Author

@rwjblue / @asakusuma do you know if the original issue:

Ensure that engines are destroyed before top-level views so that top-level views are not re-instantiated during engine teardown.

Had tests to catch a regression? Because I looked into fixing this and just switching the order back seems to pass without failing any tests.

@rwjblue
Copy link
Member

rwjblue commented Sep 10, 2016

@trentmwillis - Yeah, I think the isDestroying guard added in #14123 actually fixed the regression (just before the PR changing the order landed).

@trentmwillis
Copy link
Member Author

@rwjblue okay, so do you think switching the order back seems like the proper solution? If so, I'll update my PR with the failing test.

@rwjblue
Copy link
Member

rwjblue commented Sep 10, 2016

@trentmwillis - Yeah, lets switch it back and confirm tests pass.

@trentmwillis
Copy link
Member Author

Fixed via #14237. Tested against the latest release build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants