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 beta] Removes this._environment
guard
#14079
[BUGFIX beta] Removes this._environment
guard
#14079
Conversation
Let me know if there's something I missed in order to close #14029. Thanks! |
@duggiefresh it is missing a regression test, but I think this maybe related to: #14085 |
@@ -1232,7 +1232,7 @@ let Route = EmberObject.extend(ActionHandler, Evented, { | |||
|
|||
this.setupController(controller, context, transition); | |||
|
|||
if (!this._environment || this._environment.options.shouldRender) { | |||
if (this._environment.options.shouldRender) { |
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.
woooooo
@chancancode seemed to suggest this was good to roll if no tests failed when it was removed 8e317ee#commitcomment-18526477 @krisselden I think you're correct, are you asking for a regression test to be added here though? I wasn't sure reading your comment. |
Yes, bug fixes require test to ensure they don't regress |
6e136c2
to
282f844
Compare
Thanks for clarifying @mixonic and @krisselden. I've added a test and included renderers into |
☔ The latest upstream changes (presumably #14135) made this pull request unmergeable. Please resolve the merge conflicts. |
looks good, thank you, needs a rebase though. |
Fixes emberjs#14029 Related to emberjs#14085
282f844
to
aadb2e0
Compare
Rebased! 🍨 |
Thanks @duggiefresh! |
Fixes #14029