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

Possible Regression: Query params alias behavior changed? #12102

Closed
wagenet opened this issue Aug 14, 2015 · 13 comments
Closed

Possible Regression: Query params alias behavior changed? #12102

wagenet opened this issue Aug 14, 2015 · 13 comments

Comments

@wagenet
Copy link
Member

wagenet commented Aug 14, 2015

In 1.13.8 I had the following code:

// controller.js
  queryParams: {
    displayMode: "mode"
  }

// route.js
  queryParams: {
    mode: {
      replace: true
    }
  }

However, this started giving an error in 2.0:

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

Changing the route to the following solved the issue.

  queryParams: {
    displayMode: {
      replace: true
    }
  }
@rwjblue
Copy link
Member

rwjblue commented Aug 14, 2015

Can you provide a reproduction ember-twiddle / JSBin ?

@SaladFork
Copy link
Contributor

Reproduction:

@KTKate
Copy link

KTKate commented Aug 25, 2015

Not sure if this is the same or something different but here is a Twiddle showing the queryParams alias doesn't work at all in 1.13.8. Maybe that's why there was no error in 1.13.8.

If you have:

 queryParams: ['color', {perPage: 'per_page'}, {firstName: 'first_name'}],

Then per_page and first_name is never used.

http://ember-twiddle.com/5e1e9fe67b616837bf97

@lolmaus
Copy link
Contributor

lolmaus commented Sep 2, 2015

Possibly related: #11592.

@pixelhandler
Copy link
Contributor

another related/duplicate issue: #11977

@v3ss0n
Copy link

v3ss0n commented Jun 26, 2016

same problem. Still in 2.6
SO it is a bug?

@raytiley
Copy link
Contributor

I think #13273 will address this /CC @rwjblue

@rwjblue
Copy link
Member

rwjblue commented Jun 27, 2016

Thanks @raytiley. I'm going to close this for now, but I would love to land a new query params test to ensure we do not regress here in the future. Feel free to ping me if you would like help on how that might look...

@rwjblue rwjblue closed this as completed Jun 27, 2016
@v3ss0n
Copy link

v3ss0n commented Jul 1, 2016

so this is fixed in beta 2.7 merged ?

@rwjblue
Copy link
Member

rwjblue commented Jul 1, 2016

Please test to confirm, but yes that is what we believe.

@v3ss0n
Copy link

v3ss0n commented Jul 1, 2016

ok i will test when 2.7 released , can't upgrade coz the project with 2.6 version is close to release now.

@swquinn
Copy link

swquinn commented Sep 10, 2016

Maybe I'm not understanding the mapping of query parameters correctly or I have it backwards, but on 2.7 I have two routes (topic and reply) which are registered with the router with reply being below topic, e.g.:

  ...
  this.route('topic', { path: ':topic' }, function() {
    this.route('reply', { path: 'reply' });
  });
  ...

And encounter this assertion regularly if topic and reply share the same query parameter, in this case post. On the topic route/controller post should "jump" the user to the post in question and on reply should supply quoted text to pre-fill in the reply's text field. After first receiving the warning:

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

I tried changing my query parameter definitions to the following:

// routes/community/forum/topics/topic.js
  queryParams: {
    post: { refreshModel: true }
  }

// routes/community/forum/topics/topic/reply.js
  queryParams: {
    post: { refreshModel: true }
  }

... and the controllers to:

// controllers/community/forum/topics/topic.js
  queryParams: [
    { viewPost: 'post' }
  ]

// controllers/community/forum/topics/topic/reply.js
  queryParams: [
    { replyPost: 'post' }
  ]

But still receive:

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

My understanding of the guide, in particular the section on mapping a controller's property to a different query param key, was that within the controller I am aliasing a bound property to a query parameter, e.g. viewPost and replyPost both bind to the post query parameter. This doesn't seem possible? The error seems to suggest I can't have two routes which use the same query parameter if they are related hierarchically to one another.

The suggested solution by the assertion is less than desirable since it sounds like the solution is to use two different query parameters, e.g. ?viewPost=123 and ?replyPost=1234 which isn't really ideal.

@omairvaiyani
Copy link

In case anyone else is still having this issue (I sure just did), here's how I fixed it.

Just upgraded to 2.10.2 when this assertion came up.

Assertion Failed: You're not allowed to have more than one controller property map to the same query param key, but both ...

Turns out, if a controller has queryParams defined, it cannot be used by multiple routes, or be extended. Or at least, that's how I managed to resolve the error.

E.g. HomeController had query params, HomeIndexRoute had the property controllerName: 'home. Instead, I now made HomeIndexController and injected HomeController into it to utilise the main controller logic.

Hope that helps others coming here from Google!

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