More robustly fully scope query params

[alisdair]

Fixes #14010.

Unfortunately I can't add a test for this change, as I don't have a reliable reproduction for the bug. But in production I'm seeing exceptions indicating that queryParams is undefined in some cases, and it seems reasonable for this function to be robust to that case by shortcut-return.

[rwjblue]

This may be correct (it is somewhat hard to reason about), but I'm hesitant to land this without a regression test.

[alisdair]

@rwjblue Thanks for the poke. I've added a test that I hope makes sense.

[supersabillon]

Tried this fix on our project using 2.7 and it worked!

[rwjblue]

Does this error only occur when explicitly passing undefined? Would the following also fail:

this.transitionTo('bar');

// or
this.transitionTo('bar',  {});

?

[alisdair]

Yes, the error only seems to occur when passing undefined. I still don't know what elsewhere in the system is causing something to call transitionTo('…', { queryParams: undefined }), but that's the cause of all the app errors.

Here's the code which is trying to guess if we have queryParams or not:

let possibleQueryParams = args[args.length - 1];
if (possibleQueryParams && possibleQueryParams.hasOwnProperty('queryParams')) {
  queryParams = args.pop().queryParams;
} else {
  queryParams = {};
}

So:

• this.transitionTo('bar') works fine without my change, because possibleQueryParams is falsy and so we hit the second block of the if
• this.transitionTo('bar', {}) also hits the second block of the if, because {} fails the second condition, but then (for unrelated reasons) then results in "Error: More context objects were passed than there are dynamic segments for the route: bar".

[rwjblue]

Hmm. I would really like to understand what is actually causing the queryParams object to be undefined....

[alisdair]

Me too, but I have no idea how I can possibly find that out. I've looked through the router code as closely as I can, and haven't found any possible explanation. Nor have I found any reproduction in development. Any suggestions?

Is understanding the root cause a blocker to getting this merged? I know it's a band-aid for the underlying problem, but the code-path causing this crash still seems like incorrect behaviour to me, even if we don't know what's triggering it.

[alisdair]

Digging through a few stack traces a little further, it looks like the invalid queryParams values are coming from link-to component instances.

In _invoke, we pull values out of queryParams before passing the result to transitionTo:

let queryParams = get(this, 'queryParams.values');

But in didReceiveAttrs our fallback value of queryParams is {}. I think that means that get(this, 'queryParams.values') would return undefined in this case.

I have no idea what is happening in the internals here. I don't know what queryParams.values refers to, and I don't know how to find out. But is that possibly the underlying source of the problem?

[rwjblue]

replaced by #14068