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

QP only transition in beforeModel of application route throws error #11563

Open
raytiley opened this issue Jun 26, 2015 · 7 comments
Open

QP only transition in beforeModel of application route throws error #11563

raytiley opened this issue Jun 26, 2015 · 7 comments

Comments

@raytiley
Copy link
Contributor

Reproduction: http://emberjs.jsbin.com/verocu/2/edit?html,js,output

Our app does some beforeModel checking of data to see what "location" the user is operating. If know query param for this is provided call transitionTo in order to force the user is at a location.

Because of some other logic I just added a catch to the promise chain that the transitionTo is a part of. Now I'm noticing the error shown in the above jsbin.

Interesting thing when trying to recreate is that if you have no catch on the promise chain and the QP is set refreshTrue then a second transition will fire, will have the correct query param and so the user won't notice the failure.

@trek trek self-assigned this Jun 29, 2015
@mattmarcum
Copy link

mattmarcum commented Apr 28, 2016

👍 I created two failing tests for this

https://github.com/emberjs/ember.js/compare/master...mattmarcum:transition-to-in-before-model-with-query-params-test?expand=1

I went ahead and created failing tests in both the default code path and in the ember-routing-route-configured-query-params code path (which you can run in the tests by turning that feature flag on).

This also fails in the model hook, I've only tried those two hooks so far.

Anyone know whats up with that feature-flag? Its not in the FEATURES.md yet...

@mattmarcum
Copy link

This bug is pretty tricky because its caused by interaction between tildeo/router.js and ember.js/route.

When router.js is doing a queryParamTransition it generates an empty Transition because it should be a no-op transition. But ember.js/route needs to know the leaf node so that it can lookup the query params on the controller and do all its magic. When ember.js/route#finalizeQueryParamChange finally gets called, the transition passed to it contains no information about the current route, transition.state.handlerInfos is an empty array.

So it really seems like the fix should go in tildeo/router.js, probably around this line: https://github.com/tildeio/router.js/blob/master/lib/router/router.js#L126

If we did something like:

newTransition.state.handlerInfos = newState.handlerInfos;

I think that might fix it, but I'm not sure if that would break anything else;

@mattmarcum
Copy link

mattmarcum commented Apr 28, 2016

Actually that doesn't fix it. Even if you get the handlerInfos there's still some problems with getting the controller

@mattmarcum
Copy link

This is definitely caused by the fact that route.setup has not been called yet, it's not called until after the beforeModel hook. finalizeQueryParams expects that the route has already been setup.

Got to love the comments here: https://github.com/emberjs/ember.js/blob/v2.5.0/packages/ember-routing/lib/system/route.js#L1182-L1183

@rwjblue
Copy link
Member

rwjblue commented Dec 8, 2018

I can't tell if this is still an ongoing issue. Anyone have time to create a demo app or twiddle to confirm?

@buschtoens
Copy link
Contributor

buschtoens commented Oct 29, 2020

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);

https://github.com/tildeio/router.js/blob/d885da22340da98dcadb45427766efade54ce832/lib/router/router.ts#L123

@sly7-7
Copy link
Contributor

sly7-7 commented Mar 6, 2021

@rwjblue I think this specific use case is working on the last release (3.25.1), because this looks a lot like this: #17494 (comment)

Update, seems like I spoke too fast. I've reproduced the use case of the original @raytiley example here: https://github.com/sly7-7/repro-ember-17494/tree/derived-from-ember-11563

It appears that on current release, the error is catched, but after closing the alert, the state seems good.
So I've tested with a potential fix (tildeio/router.js#303), and the error is still here, but the execution is the stopped.
In both cases, as noted by @raytiley, not catching the error will result at the end in a "good state", but the error shows in the console
Screenshot from 2021-03-06 11-05-52

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

7 participants