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

Redirecting a transition to same route with different query params does not work unless transition is aborted before #18577

Open
jelhan opened this issue Nov 25, 2019 · 18 comments · May be fixed by tildeio/router.js#307

Comments

@jelhan
Copy link
Contributor

jelhan commented Nov 25, 2019

I'm seeing two weird bugs if doing a redirect to the same route with different queryParams in beforeModel hook:

  1. If the route is entered via <LinkTo> or transitionTo(), the query param is not reflected in the URL.
  2. If the route is entered directly on application start, ember throws an error.

The error thrown is this one:

TypeError: Cannot read property 'name' of undefined
    at Class._queryParamsFor (https://t95pf.sse.codesandbox.io/assets/vendor.js:35740:59)
    at Class.finalizeQueryParamChange (https://t95pf.sse.codesandbox.io/assets/vendor.js:34759:29)
    at Class.triggerEvent (https://t95pf.sse.codesandbox.io/assets/vendor.js:36198:27)
    at PrivateRouter.triggerEvent (https://t95pf.sse.codesandbox.io/assets/vendor.js:35078:43)
    at PrivateRouter.finalizeQueryParamChange (https://t95pf.sse.codesandbox.io/assets/vendor.js:72248:12)
    at PrivateRouter.queryParamsTransition (https://t95pf.sse.codesandbox.io/assets/vendor.js:71741:37)
    at PrivateRouter.getTransitionByIntent (https://t95pf.sse.codesandbox.io/assets/vendor.js:71810:37)
    at PrivateRouter.transitionByIntent (https://t95pf.sse.codesandbox.io/assets/vendor.js:71759:21)
    at PrivateRouter.doTransition (https://t95pf.sse.codesandbox.io/assets/vendor.js:71894:19)
    at PrivateRouter.transitionTo (https://t95pf.sse.codesandbox.io/assets/vendor.js:72372:19) 

It's caused by routeInfos of transition being an empty array here: https://github.com/emberjs/ember.js/blob/v3.14.1/packages/%40ember/-internals/routing/lib/system/route.ts#L2523 _queryParamsFor can't handle that case and throws at this line: https://github.com/emberjs/ember.js/blob/v3.14.1/packages/%40ember/-internals/routing/lib/system/router.ts#L926

For a minimal reproduction use this code:

// app/routes/foo.js

import Route from '@ember/routing/route';

export default Route.extend({
  async beforeModel(transition) {
    if (!transition.to.queryParams.bar) {
      await this.transitionTo('foo', { queryParams: { bar: "1" }});
    }
  }
});
// app/controllers/foo.js

import Controller from '@ember/controller';
import { inject as service } from '@ember/service';

export default Controller.extend({
  router: service(),

  queryParams: ['bar'],
});
// app/templates/foo.hbs

{{log router.currentRoute}}

The current route is logged in template to verify that the query params are available on RouteInfo object for scenario 1. And they are indeed. They are just not represented in URL.

Both bugs could be fixed by explicitly aborting the transition before doing the redirect:

// app/routes/foo.js

import Route from '@ember/routing/route';

export default Route.extend({
  async beforeModel(transition) {
    if (!transition.to.queryParams.bar) {
      transition.abort();

      await this.transitionTo('foo', { queryParams: { bar: "1" }});
    }
  }
});

But accordingly to documentation transitionTo() should implicitly abort the transition. It should not be required to do it manually. Also if this is the recommended solution, there should be an assertion if the transition is not aborted before. It should not end with a broken state (URL not reflecting query parameters in URL) neither with throwing a TypeError.

There are some issues which seem to be related but slightly different:

@acorncom
Copy link
Contributor

@jelhan which version of Ember are you seeing this in?

@jelhan
Copy link
Contributor Author

jelhan commented Jan 21, 2020

@acorncom I'm quite sure that I had reproduced it using the latest version of Ember at the same day when reporting. So I think it was 3.14.1. But I was also able to reproduce it with latest ember-source available today, which is 3.16.0.

I created a repository with the reproduction mentioned above: https://github.com/jelhan/ember-demo-redirect-to-same-route-with-different-query-params

Here are the steps to reproduce both issues mentioned above using that repo. Assuming that you have cloned it, installed dependencies and run ember serve.

1. transition to route triggering the redirect from another route

Expected:

  • URL should be http://localhost:4200/foo?bar=1
  • currentURL property of RouterService should be /foo?bar=1
  • currentRoute.queryParams should deep equal { bar: 1 }

Actual:

  • URL is http://localhost:4200/foo (missing query param)
  • currentURL property of RouterService is /foo (missing query param)
  • currentRoute.queryParams deep equals { bar: 1 } (correct but out of sync with URL)

2. open that route directly

See an error been thrown and nothing rendered. The error is:

TypeError: routeInfos[(routeInfoLength - 1)] is undefined

It's caused by routeInfos of transition being an empty array here: https://github.com/emberjs/ember.js/blob/v3.14.1/packages/%40ember/-internals/routing/lib/system/route.ts#L2523 _queryParamsFor can't handle that case and throws at this line: https://github.com/emberjs/ember.js/blob/v3.14.1/packages/%40ember/-internals/routing/lib/system/router.ts#L926

Fixing the error by aborting the transition

To reproduce that aborting the transition before doing the redirect fixes both issues uncomment these line: https://github.com/jelhan/ember-demo-redirect-to-same-route-with-different-query-params/blob/1326804684273b9369725099f36f262fabdd9832/app/routes/foo.js#L6-L7

@chriskrycho
Copy link
Contributor

I have what appears to be an additional variant of this, available in this repo.

If you try to do a transitionTo or replaceWith in a route's afterModel, and it's a query-params-only transition, and it's the first time entering the application (i.e. first render), it will fail with exactly the error @jelhan reported above. Moreover, transition.abort() does not fix things in this case; it just leaves you with a dead app. 😬

To see the reproduction behavior and how it varies for later entries, comment out these lines: later transitions work fine; only the first transition into the route fails this way. To see that transition.abort() only further hoses things in this case, comment those lines back out and uncomment this line.

@xg-wang
Copy link
Contributor

xg-wang commented Jul 24, 2020

@chriskrycho I think I just ran into the bug you're describing, but want to confirm.
I'm seeing 404 when clicking your link. Do you still have the repo somewhere?

@chriskrycho
Copy link
Contributor

Fixed! I accidentally made a private repo. Should be visible now!

@snewcomer
Copy link
Contributor

Same...Here is my repo.

beforeModel() {
  // flawed logic through a refactor that eventually lead to this
  // route was "a-route.index"
  this.router.transitionTo(sameRouteAsIAmOn);
}

@buschtoens
Copy link
Contributor

Cross-posting #11563 (comment):

I just hit this as well in Ember 3.14.3, but I doubt that latest has fixed the bug. I believe that this is related to #12169.

I also believe that I found a "fix", but tbh I have no clue what's going on with router.js. 🤷‍♂️

- let newTransition = new InternalTransition(this, undefined, undefined);
+ let newTransition = new InternalTransition(this, undefined, newState);

tildeio/router.js@d885da2/lib/router/router.ts#L123

@jelhan
Copy link
Contributor Author

jelhan commented Oct 29, 2020

I updated my reproduction repository to Ember 3.22. Can still reproduce the bug with that latest stable release.

The error message thrown in the second case changed a little bit. It's now: TypeError: Cannot read property 'name' of undefined. But I didn't noticed any change of the actual situation. Maybe Chrome just changed the behavior for array[-1]?

The full stack trace to help people finding this issue with Google:

Error while processing route: foo Cannot read property 'name' of undefined TypeError: Cannot read property 'name' of undefined
    at Router._queryParamsFor (http://localhost:4200/assets/vendor.js:23164:59)
    at Class.finalizeQueryParamChange (http://localhost:4200/assets/vendor.js:22188:29)
    at Router.triggerEvent (http://localhost:4200/assets/vendor.js:23622:27)
    at PrivateRouter.triggerEvent (http://localhost:4200/assets/vendor.js:22506:43)
    at PrivateRouter.finalizeQueryParamChange (http://localhost:4200/assets/vendor.js:63090:12)
    at PrivateRouter.queryParamsTransition (http://localhost:4200/assets/vendor.js:62580:37)
    at PrivateRouter.getTransitionByIntent (http://localhost:4200/assets/vendor.js:62652:37)
    at PrivateRouter.transitionByIntent (http://localhost:4200/assets/vendor.js:62601:21)
    at PrivateRouter.doTransition (http://localhost:4200/assets/vendor.js:62736:19)
    at PrivateRouter.transitionTo (http://localhost:4200/assets/vendor.js:63214:19)

I applied @buschtoens patch. This resolves the original bug but reveals another one: The to property of transition passed as first argument to beforeModel hook is undefined.

@rreckonerr
Copy link
Contributor

@jelhan I've seen the same error while working on an issue related to disappearing QP's!
Can you please check if @buschtoens's tildeio/router.js#307 and mine tildeio/router.js#308 combined solve the issue? I believe it might help since we both fix changes brought in by the same commit 🤔🤔🤔.

@jelhan
Copy link
Contributor Author

jelhan commented Nov 25, 2020

@rreckonerr If I got it right you suspect that the bug was introduced by tildeio/router.js@f385d11. It was released in router_js@5.0.0.

Ember.js upgraded to router_js@^5 here: #17007

ember-source@3.6.0 is the first stable release containing this version:

"router_js": "^6.1.3",
The stable release before ember-source@3.5.1 as using router_js@2.0.0-beta.4:
"router_js": "2.0.0-beta.4",

Will try to find some time to verify that the bug is reproducible with ember-source@3.6.0 but not with ember-source@3.5.1. The ember-source@3.6.0 contains many router related changes. But at least it would help to trace down the bug to that big router refactoring.

@kmccullough
Copy link

I am experiencing the same issue where an exception is thrown when the application is loaded on the route which is transitioning only to change the query-params. It's a very simple reproduction in 3.22: https://github.com/kmccullough/ember-default-query-param

@sly7-7
Copy link
Contributor

sly7-7 commented Mar 5, 2021

@kmccullough Thanks for this example :)
Locally, with the recent changes on the router, I don't encounter your error anymore. :)
Capture d’écran 2021-03-05 à 14 40 28

@jelhan There is no more error with the last changes, but the behavior is still inccorrect, I mean the qp update is not reflected in the URL. I'll try to investigate. Thanks to the works done by @alexlafroscia and @buschtoens maybe this will be resolved.

Capture d’écran 2021-03-05 à 14 59 25

@patrickberkeley
Copy link

I'm seeing this in ember 3.26.1 as well.

@panthony
Copy link

Same problem as @chriskrycho with ember 3.22.

Also cannot use transition.abort() because it breaks the app when it's a first render.

@YoranBrondsema
Copy link
Contributor

We're still seeing this issue in Ember.js 5.1.

If we're replacing a route with the same route but different query params:

export default class PortfolioRoute extends Route {
    beforeModel() {
        this.router.replaceWith('portfolio', {
          queryParams: {
            bla: 'bla'
          }
      });
    }
}

the route crashes on first render with:

router.js:836 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'name')
    at Router._queryParamsFor (router.js:836:1)
    at ApplicationRoute.finalizeQueryParamChange (route.js:1242:1)
    at Router.triggerEvent (router.js:1219:1)
    at PrivateRouter.triggerEvent (router.js:234:1)
    at PrivateRouter.finalizeQueryParamChange (router_js.js:1703:1)
    at PrivateRouter.queryParamsTransition (router_js.js:1257:1)
    at PrivateRouter.getTransitionByIntent (router_js.js:1317:1)
    at PrivateRouter.transitionByIntent (router_js.js:1275:1)
    at PrivateRouter.doTransition (router_js.js:1399:1)
    at PrivateRouter.transitionTo (router_js.js:1809:1)

Unfortunately none of the workarounds seem to work. Any suggestions on getting around the bug?

@rassloff
Copy link

rassloff commented Jul 6, 2023

same problem in my project. any suggestions ???

@seanCodes
Copy link

seanCodes commented Aug 21, 2023

Just encountered this on Ember 4.12. 😞 @buschtoens fix in tildeio/router.js#307 does seem to fix it for me, though. Pretty amazed this has gone so long without being addressed since it seems like a common scenario.

Anyone know of any workarounds or any way this could be monkey-patched?

@Mikek2252
Copy link
Contributor

Mikek2252 commented Aug 28, 2023

@seanCodes I have been investigating this bug and also found that if i was trying to transition multiple times it would also trigger e.g.

beforeModel() {
  if (foo) {
    this.transitionTo('this.route', { queryParams });
  }
  if (bar) {
    this.transitionTo('this.route', { queryParams });
  }
}

By adding returning on transition it stopped the error.

beforeModel() {
  if (foo) {
    return this.transitionTo('this.route', { queryParams });
  }
  if (bar) {
    return this.transitionTo('this.route', { queryParams });
  }
}

Maybe this might help in your case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.