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

transitionTo queryParams on initial render still broken #14875

Open
zidjian257 opened this issue Jan 25, 2017 · 30 comments
Open

transitionTo queryParams on initial render still broken #14875

zidjian257 opened this issue Jan 25, 2017 · 30 comments

Comments

@zidjian257
Copy link

zidjian257 commented Jan 25, 2017

The problem described in #12169 still exists, so I'm filing a new ticket, as the initial ticket is already closed.

@zidjian257
Copy link
Author

zidjian257 commented Jan 25, 2017

the only aproach to get around this in ember 2.10 for me was the solution posted by @taras on 23 Dec 2015 in #12169 (the other solutions mentioned didn't work):

let RedirectAfterDidTransition = Ember.Mixin.create({
  redirectAfterDidTransition(...args) {
    this.router.one('didTransition', ()=>{
      this.router.transitionTo(...args);
    });
  }
});

export default Route.extend(RedirectAfterDidTransition, {
  afterModel() {
    this.redirectAfterDidTransition('index');
  }
});

@Serabe
Copy link
Member

Serabe commented Jan 27, 2017

Updated jsbin to Ember 2.11.
Updated to 2.12 beta 1 and seems fixed can you provide a test that breaks under 2.11 but is fixed in 2.12 to make sure we don't have a regression?

Thank you!

@rafaelbnp
Copy link

+1 on this. Having same issue!

@GCheung55
Copy link
Contributor

Seeing this on 2.10.2. Here's the stacktrace:

TypeError: Cannot read property 'name' of undefined
    at Class._queryParamsFor (vendor.js:47541)
    at Class.finalizeQueryParamChange (vendor.js:45361)
    at Router.triggerEvent (vendor.js:47954)
    at trigger (vendor.js:75664)
    at finalizeQueryParamChange (vendor.js:77652)
    at Router.queryParamsTransition (vendor.js:76999)
    at Router.getTransitionByIntent (vendor.js:76908)
    at Router.transitionByIntent (vendor.js:77017)
    at doTransition (vendor.js:77619)
    at Router.transitionTo (vendor.js:77093)

@kbullaughey
Copy link

I'm seeing this on 2.13.

@akaravashkin
Copy link

+1 on 2.13.3

@jemware
Copy link

jemware commented Jul 13, 2017

I’m seeing this in 2.14.0

@gschulze
Copy link

Running into the same issue on 2.15.0.

@divmgl
Copy link

divmgl commented Dec 12, 2017

Ember 2.16.2 still has this issue

@gschulze
Copy link

So this still happens on 2.18.0. Really?

@bretjb
Copy link

bretjb commented Feb 28, 2018

@gschulze I am seeing this on 2.18.

@alvinvogelzang
Copy link

I'm still experiencing this issue as well in Ember 2.18

@pixelhandler
Copy link
Contributor

@GCheung55 @akaravashkin @alvinvogelzang @bretjb @divmgl @gschulze @jemware @kbullaughey @locks @rafael-paiva @zidjian257 Just curious is this still an issue in Ember 3?

@rafaelbnp
Copy link

@pixelhandler I'm still on the LTS 2.18.2 and I don't have the time now to check Ember 3... sorry

@emberjs emberjs deleted a comment from turino2018 Sep 21, 2018
@pixelhandler
Copy link
Contributor

@rafael-paiva thanks for the update :)

@pixelhandler pixelhandler changed the title trasitionTo queryParams on initial render still broken transitionTo queryParams on initial render still broken Sep 21, 2018
@ohcibi
Copy link

ohcibi commented Oct 14, 2018

I'm seeing this on 3.4.1 but I cannot reproduce it in a twiddle. I get the very same stacktrace mentioned above and I found that in the line the exception is thrown, the problem is that that .name property is tried to get from on object out of an empty array. I'd say there has to be a check if that array is empty which throws a better error message. I created #17118 to address this specifically.

@teakap
Copy link

teakap commented Oct 17, 2018

I am using 2.18.2 and had the same error. Canceling the transition from the transition object did the trick for me. My use case was handling a 401 on the error hook.

error: function (error, transition) {

  if (error.status === '401') {
    // Do other stuff ex. localStorage.clear();
    transition.abort();
    this.transitionTo('index', {
       queryParams: {
           showLogin: true
       }
     });

    return true;
  }
}

@ming-codes
Copy link
Contributor

ming-codes commented May 29, 2019

I'm seeing this in 3.10 😕

@JackEllis
Copy link

Duplicate of #17118 I believe.

Ran into this issue and it didn't play nicely with next(function()) because it meant that components would render before the new tansition.

Solution that worked for me:

import Route from '@ember/routing/route';
import { run } from '@ember/runloop';

export default Route.extend({
    queryParams: {
        site: {
            refreshModel: true,
            replace: true
        }
    },

    beforeModel(transition) {
        if (isEmpty(transition.queryParams.site)) {
            run(this, function() {
                this.replaceWith({ queryParams: {
                    site: 1,
                }});
            });
        }
    },

    model(params) {}
});

So using 'run' instead of 'next' brought it in ahead of the component rendering etc. I noticed this because I was seeing extra XHR requests firing off before we had a valid site ID

@lolmaus
Copy link
Contributor

lolmaus commented Sep 12, 2019

@JackEllis Why not return this.replaceWith() without run/next? And maybe transition.abort(), not sure.

@JackEllis
Copy link

@lolmaus Because it doesn't work without run / next. If I return it without the run / next we get the error this thread is about

@lvegerano
Copy link

This is happening in 3.12

@arthur5005
Copy link

arthur5005 commented Nov 7, 2019

And 3.13
This might be a superficial assessment, but this method in router class seems to allow for unsafe access:

_queryParamsFor(routeInfos: PrivateRouteInfo[]) {
let routeInfoLength = routeInfos.length;
let leafRouteName = routeInfos[routeInfoLength - 1].name;
let cached = this._qpCache[leafRouteName];
if (cached !== undefined) {
return cached;
}

What's happening is routeInfos collection is coming in empty, which I imagine is a problem in and of it self (I don't know enough about the internals to comment), but if we changed these two lines to allow for safe access, at least an exception wouldn't be thrown in the the case where routeInfos is an empty array.

let leafRouteName = routeInfos[routeInfoLength - 1].name;
let cached = this._qpCache[leafRouteName];

@ohcibi
Copy link

ohcibi commented Nov 11, 2019

@JackEllis I created #17118 as I debugged this error up to the point @arthur5005 showed (let leafRouteName = routeInfos[routeInfoLength - 1].name) and also concluded that the problem is that routeInfos` is empty. I think this error is way to obvious that it got overlooked and was expecting this to cause errors in other situations as well but since I found nothing, I created the other issue to gain more knowledge about that code; why it is written that way and if a fix could be as simple as catching the obvious empty array exception like @arthur5005 suggested.

This was one year ago and I haven't even got a response that confirms wether or not that piece of code is written badly by accident or if there is a reasoning behind it.

git blame mentions @chadhietala as the author of those two lines. Maybe he has some insights to it? Also @rwjblue was commenting on my issue but hasn't commented on the replies to his comments since. I'm unsure if the core team is even aware of this pretty obvious issue.

@frinyvonnick
Copy link

frinyvonnick commented Mar 12, 2020

Hi, I ran into this same issue and fixed it using redirect instead of beforeModel. Hope it will help someone 👍

@elgordino
Copy link

This is still occurring in ember 3.17.

@brokenalarms
Copy link

using redirect causes the same issue for me in 3.12 unfortunately.

@arthur5005
Copy link

arthur5005 commented Oct 22, 2020

Ran into this issue again on 3.20.4 Can anyone with expertise comment on my assessment? Happy to help with some direction.

@buschtoens
Copy link
Contributor

Can y'all check whether this fixes your issue? 😊
tildeio/router.js#307

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