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

Using controllerName with a parent controller that has a query-param causes assertion on Ember 2.10-beta.2. #14560

Open
workmanw opened this issue Oct 31, 2016 · 13 comments

Comments

@workmanw
Copy link

workmanw commented Oct 31, 2016

Our ember-try tests alerted us to a possible regression. After investigation I discovered that if a route declares a controllerName that matches a parent controller and that parent controller has query-params it will cause an assertion to be thrown when using a link-to or this.transitionTo.

This assertion only occurs on Ember-2.10-beta, it does not occur on Ember-2.9.

Here is a twiddle: https://ember-twiddle.com/515fb727fe1e8b39b2520ec3fae23bff

In this scenario we have an "invite key" for a user. Redeeming an invite is a multi-step [multi-route] process similar to most SaaS apps. The namespaced controller (controllers/invite) houses the application state for all steps in this process, including the query-param (key). Each of the route classes declare controllerName: 'invite' so their templates can interface with the invite controller directly.

The problem is that now two handlers (invite and invite.verify) are both using the same controller. When transitionTo calls into the router's _prepareQueryParams function, a newly added check throws this exception because both handlers are using the same query-params (d1124fa). The exception is also a bit non-obvious:

Assertion Failed: You're not allowed to have more than one controller property map to the same query param key, but both invite:key and invite:key map to key. You can fix this by mapping one of the controller properties to a different query param key via the as config option, e.g. `key: { as: 'other-key' }

So why use controllerName at all? Why not just use an invite service to house the state from each phase of the invite process? The answer is two parts: 1) It's old code and we haven't touched it in a long long time 😜, but 2) there is not a clean way for a service to access query-params on the router.


So yea, I know this is just another one of the crazy query-param issues that has come up. If you guys want to just ¯\_(ツ)_/¯, I'm happy with that. I can probably find my own work around. But as always I'm just reporting the issues I see.

@rwjblue
Copy link
Member

rwjblue commented Oct 31, 2016

I haven't really dug into the demo yet, but this is related to a large scale router and query param cleanup effort.

Most likely @trentmwillis will have a better understanding of exactly what is going on here, and can advise to it being a "bug" or a new "feature" ( 😺 ).

@trentmwillis
Copy link
Member

I'll have to dig in more later, but this assertion isn't really new, it was just moved to a different part of the QP flow.

My guess is that previously it wasn't checking this specific QP due to missing from the hash in the serialization step. I'll try to dig in more later, but one clarifying question: when we use controllerName, do we create multiple instances of the controller or is it still just a singleton used by multiple routes? I think that will inform whether this is an actual error or not.

@workmanw
Copy link
Author

@trentmwillis It is definitely the later AFAIK. All controllers are singletons just as services are. That's what makes the reuse of the invite controller via controllerName so valuable (multiple routes setting data in a single place).

For reference, here is the api docs: Ember.Route#controllerName.

@trentmwillis
Copy link
Member

Okay. If it's the latter, then this should be considered a bug. Should be an easy fix though, we should just improve the check here to see if the QPs have the same controllerName and same prop, if so then we shouldn't error. This is only an error if the QPs use the same urlKey and come from different properties/controllers.

@pixelhandler
Copy link
Contributor

@workmanw I do find it odd using the same controller for both routes. Would it be possible to inject a service into all the routes and controllers that need it? Then use the route's setupController method to provide the properties the various templates need?

@workmanw
Copy link
Author

workmanw commented Nov 4, 2016

@pixelhandler Yea I talked about that as a workaround strategy in my initial report. The core reason that I hadn't done that was because of the query-params. There isn't a good option for a service to "cleaning" get and set query-params. Using a controller is just so much more natural when query-params are involved. But if this issue is ultimately deemed as something we won't fix, they I'll switch over to using a service and just use private APIs to talk to the router.

EDIT: BTW, if it's not clear, I'm more than happy to use my workaround. I agree that this is a little weird already. I've submitted other weird issues like this in the past. Part of this just comes from having a nearly 5 year old ember app (Dang, 5 years is coming up soon. 😮 ). My goal is simply to report unexpected changes we see in beta/canary builds (thanks ember-try!) to help get the ball going quicker on resolutions. I perfectly understand shedding old crufty APIs 😄.

@sohara
Copy link

sohara commented Nov 15, 2016

Just wanted to add that we are also seeing this on our main production app, moving from 2.9.1 to most 2.10.beta3. I will probably just stop sharing the same controller between two routes (inject the parent controller as a prop in the child controller).

@apezel
Copy link

apezel commented Dec 1, 2016

Any progress on this issue ? Maybe raising a warning would be more relevant because i can't get my application to work at all with ember 2.10 because of this issue.

@kbullaughey
Copy link

In issue #14835 I've posted another twiddle that @efx has pointed out may reproduce the same problem as described in this issue. I've left that issue open in case anyone want's to use the twiddle to verify that the fix also resolves that problem.

@pixelhandler
Copy link
Contributor

@workmanw @trentmwillis @rwjblue is this still an issue, perhaps we should close; what do you think?

@workmanw
Copy link
Author

@pixelhandler I'm no longer using Ember so I can't say for sure. But I had been working around this issue for a while. I'm guessing it's okay to close it.

@turino2018
Copy link

turino2018 commented Sep 15, 2018 via email

@cafreeman
Copy link
Contributor

Hi there! We are currently experiencing this exact issue on a 3.4 app. I've opened a failing test PR with an included twiddle and description here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

9 participants