-
Notifications
You must be signed in to change notification settings - Fork 159
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] provide transition to setupContext for internal transit… #308
[BUGFIX beta] provide transition to setupContext for internal transit… #308
Conversation
c52be85
to
26765a3
Compare
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.
Generally seems good, but we will need a test (that failed previously, and passes now) in order to confirm and to ensure we don't accidentally regress in the future...
@rwjblue Do we need a test case in this repo or in ember.js? I've already checked that this change fixes failing reproduction in ember.js emberjs/ember.js#16986 |
I'd say both actually. |
…ions Prior tildeio@e792f2c we were calling `this.setupContext(newState, transition)` for intermediate transitions, which got replaced with `this.setupContexts(newState)`. This results in buggy behavior when query params get lost in certain scenarios. Refs: emberjs/ember.js#14438
26765a3
to
0be0d19
Compare
@rwjblue I've prepared a PR in ember.js repo with failing tests fixed by this PR emberjs/ember.js#19249 |
Awesome, thank you @rreckonerr! |
Just released as v7.1.1, would you mind sending an Ember PR with the previously failing test (along with the upgrade)? |
…ions
Prior e792f2c we were calling
this.setupContext(newState, transition)
for intermediate transitions, which got replaced withthis.setupContexts(newState)
. This results in buggy behavior when query params get lost in certain scenarios.Refs: emberjs/ember.js#14438
Closes emberjs/ember.js#14438 (hopefully)