Replace after router service transition adds history entry

[xe21500]

Hi,

I think we may have discovered a bug with the routerService transitions under some circunstances involving routes with query params. This is happening on Ember 2.16, but the code I traced to cause the bug is currently on master, so it should be broken on 2.18 / 3.0 too.

I've created this repo to illustrate the issue (the structure could possibly be simplified further, but this closely resembles my real life scenario): there is an index route with links to a working and broken routes, using routerService transitionTo method. Both these routes have a dynamic part (working/:id) and the one that breaks also has a query param with refreshModel=true; in their templates both let you transition to the next id using a link-to with replace=true.

Turns out that, when you access the page, press the "broken" link (which gets added to the history, thats OK as we are transitioning, not replacing), then press the "next" link, you get a second, unwanted history entry.

I've been investigating the issue in case it was in my side and finally discovered that the culprit was this condition on system router: transitioning from the service is preventing the query params from being correctly pruned of their undefined value, and these are getting saved on the library router's state property. This can cause problems further down the line with future transitions, as the query params change list reflects that some query params did change from previous state (in reality, it only checks that previous state object has property keys that the new one doesn't) and that triggers the queryParamsDidChange event on the base route which causes a refresh, in turn this creates a new transition that somehow hijacks the original one and finalizes before the original could set it's urlMethod to replace!!!

The working route is exactly the same, but it works because it doesn't have any query params and the event doesn't get triggered.

My workaround to the issue was to stop using routerService transitionTo, and instead use the internal private _router one until it gets solved. I don't really know why the aforementioned condition makes the transition do different things when coming from the service, although I'm sure it has some reason behind it. For now, I'm discouraging my coworkers from using the service.

Additionally, just as speculation but I'd say there is some race condition happening within queryParamsDidChange event; the transition spawned from the refresh shouldn't be able to finalize before the original one sets the urlMethod.

Thanks!

[Serabe]

The condition in the system router is because of this part of the RFC

It looks like getChangeList should be able to compare with and without default values or maybe be more intelligent on a different way.

I don't know enough about the router to triage this more wihtout help. Maybe @rwjblue or @locks can help us a bit more here.

[Serabe]

Forked repo https://github.com/ember-triage/ember-replace-qp-issue

[pixelhandler]

@rwjblue @xe21500 is this still an issue, perhaps we should close or create a new reproduction of this, what do you think?

[pixelhandler]

@xe21500 Can you update your example reproduction to the current release of Ember?

[xe21500]

@pixelhandler updated the test repo to 3.5.0, the issue still happens. Thank you!