Fix queryParamsOnly being correct on transitions. #198
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@mike-north this is relevant to your interests. |
|
This looks good to me, thanks for tackling it! I'll leave it open for a day or so and give others a chance to review too... |
This property has been set incorrectly on some transitions, notably when the transition is caused by the Router#refresh() method in queryParamsDidChange. This is the method ember uses to implement `refreshModel` option of query params, so fixing this bug allows `refreshModel` to be used in conjunction with `replace` option for query params, currently it is either-or.
alexspeller
force-pushed
the
fix-queryParamsOnly-property-on-transition
branch
from
437fd1a
to
9e5440b
Compare
|
@rwjblue I'm still 👍 on this. |
|
@nathanhammond good news! Thanks |
|
+1 Any update on this? |
alexspeller
referenced
this pull request
Jan 13, 2017
This has been broken forever AFAIK, and is surprising to a lot of people. In fact, even the ember guides recommend [using `this.transitionTo` for redirecting](https://guides.emberjs.com/v2.9.0/routing/redirection/) This is in fact broken. If you use `transitionTo` as a redirect strategy, then you get an extra history entry if you hit the route as the first route in your app. This breaks the back button, as the back button takes you to the route with the redirect, then immediately redirects back to the page you're on. Maybe you can go back if you hammer back button really quickly, but you have to hammer it loads super quick. Not a good UX. `replaceWith` works if you use it to redirect from a route that's your initial transition. However if you use it to redirect and you hit that route from some way _after_ the initial app load, then at the point that the replaceWith is called, you are still on the same URL. For example, if you are on `/` and you click a link to `/foo`, which in it's model hook redirects to `/bar` using `replaceWith`, at the time replaceWith is called, your current url is `/`. This means `/` entry gets removed from your history entirely. Clicking back will take you back to whatever page you were on before `/`, which often isn't event your app, maybe it's google or something. This breaks the back button again. This commit should do the correct thing in all cases, allowing replaceWith and transitionTo outside of redirects as specified by the developer but only allowing transitionTo or replaceWith in redirects in a way that doesn't break the back button.
alexspeller
added a commit
to alexspeller/router.js
that referenced
this pull request
Apr 17, 2017
So I fixed this in tildeio#198. But it was broken again by tildeio#197 and the tests didn't catch that. So I've improved the tests and fixed it again.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This property has been set incorrectly on some transitions, notably when the
transition is caused by the Router#refresh() method in queryParamsDidChange.
This is the method ember uses to implement
refreshModeloption of query params,so fixing this bug allows
refreshModelto be used in conjunction withreplaceoption for query params, currently it is either-or.Resolves emberjs/ember.js#10577 and emberjs/ember.js#11843