emberjs / emberjs/ember.js

Replace after router service transition adds history entry

Open
#16,262 5 comments 3 reactions 1 assignee Claimed by @rwjblue View on GitHub
bug Has Reproduction Routing
Dominant language
TypeScript
Stars
22.6k
Forks
4.2k
Avg merge
3d 12h
Merged PRs (30d)
15

Description

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](https://github.com/xe21500/ember-replace-qp-issue) 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.

![issue](https://user-images.githubusercontent.com/24647195/36413475-dbda4996-161e-11e8-9686-39390707656e.gif)

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](https://github.com/emberjs/ember.js/blob/master/packages/ember-routing/lib/system/router.js#L814): 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](https://github.com/tildeio/router.js/blob/master/lib/router/router.js#L397) 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`](https://github.com/emberjs/ember.js/blob/master/packages/ember-routing/lib/system/router.js#L395).

Thanks!

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.