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

router.replaceWith calls pushState on the history object with refreshModel QP #18416

Open
mehulkar opened this issue Sep 20, 2019 · 7 comments
Open

Comments

@mehulkar
Copy link
Contributor

https://github.com/mehulkar/ember-example-replace-push-state

I think this may have the same root cause as #15801, but it manifests differently.

Problem

If a queryParam is configured to refreshModel: true, calls to router.replaceWith end up calling pushState on history instead of replaceState.

I'm not totally sure how this would manifest as a "bug" in a browser for an end user, but in my case, I have a custom Location using a custom history object, and calling pushState when we really meant to replace causes it to get an extra entry history entry.

Debugging

It seems that Ember (or underlying router.js lib thinks that because of the queryParams refreshModel config, this replaceWith transition is invalid, and it cancels it and generates its own.

Because router.replaceWith is implemented like this:

replaceWith() {
  this.transitionTo(...arguments).method('replace');
}

the transitionTo() promise gets replaced, and all the async stuff that causes the URL to update happens on the new promise, which does not yet have the urlMethod set to replace.

One possible solution here is that instead of using transitionTo(), a Transition object should be constructed with urlMethod set as part of the constructor, so that it does the right thing throughout its lifecycle, but that seems like a really big change.

@mehulkar
Copy link
Contributor Author

#5566 may also be related

@mehulkar
Copy link
Contributor Author

As recommended in #5566 (comment), I'm able to solve the issue by using the private queryParamDidChange method instead

@mehulkar
Copy link
Contributor Author

mehulkar commented Oct 5, 2019

Converting to a native class in the route seems to break this workaround, so I might be back at square 1.

@alexlafroscia
Copy link

I noticed something similar in my app (it could the exact same issue but it's a little hard to tell)

I put together a Twiddle that shows the problem we are facing

https://ember-twiddle.com/39fe48997dd6e62f6381e89d3c6aa74c?openFiles=routes.first.index%5C.js%2C

Essentially, the flow of the logic is

  1. Click on a link to a route
  2. A route within that path does a replaceWith to set some default query params
  3. Further down the routing "stack", another replaceWith is performed to redirect to different route within the routing hierarchy of the parent that added the query param

I would expect that the browser's history receives a single pushState with the final URL, at the "child" route, with the query param.

However, what actually happens is that an intermediary URL is pushed into the browser with replaceState, meaning that the location that the browser actually "lands" has a back button pointing to that intermediary state.

@alexlafroscia
Copy link

This is still happening as of Ember 3.20.4.

I'm not totally sure how this would manifest as a "bug" in a browser for an end user

For our users, it means that they get stuck in a loop where they are unable to use the Back button anymore; doing do only removes the query param from the URL, which then re-enters the logic that calls replaceWith again to add the query param back in again.

I'm going to take a stab at adding a failing test to the Ember codebase for this.

bors added a commit to rust-lang/crates.io that referenced this issue Oct 4, 2020
sentry: Ignore `TransitionAborted` errors

According to emberjs/ember.js#18416 it looks like changing query params that have `refreshModel: true` causes an internal `TransitionAborted` error. This error is being ignored by Ember itself (see https://github.com/emberjs/ember.js/blob/v3.21.3/packages/`@ember/-internals/runtime/lib/ext/rsvp.js#L40-L42),` but Sentry also hooks into `RSVP.on('error')` and unfortunately reports these false positives (see emberjs/ember.js#12505)

This PR actively ignores `TransitionAborted` errors in the `beforeSend()` hook of Sentry.

r? `@jtgeibel`
@frykten
Copy link

frykten commented May 24, 2021

Hi!

We had the same issue recently but not with query-params.

We are using the replaceWith methods to make a feature with sub-routes feels like it is only one history entry, hence playing with the replace to navigate.

  • As long as navigation is triggered from the UI, Ember works normally and well;
  • When navigating from the browser (eg. browser navigation, back and forward arrows, using history.back(), or URLs), however, replaceWith, instead of making a replaceState, makes a pushState. In the console, it looks really close to what the reporter of this thread showed.

Also, seems like it could be related to what's written in the documentation (page is preventing-and-retrying-transitions):

However, if the browser back button is used to navigate away from route:form, or if the user manually changes the URL, the new URL will be navigated to before the willTransition action is called. This will result in the browser displaying the new URL, even if willTransition calls transition.abort().

To prevent the pushState, we found a workaround to abort the browser-triggered transition, and create a new one right away from Ember... And then the replaceWith works as intended.

@mattmarcum
Copy link

Just ran into this issue on a big production app. Its definitely something that customers can encounter, exactly like @alexlafroscia described above over a year ago

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

6 participants