[FEATURE query-params-new] Query params rewrite

[machty]

See here for some examples: https://gist.github.com/machty/8167051

Depends on Handlebars subexpressions:
handlebars-lang/handlebars.js#690

[machty]

Build is failing due to unmerged Handlebars dependency wrt subexpressions

[ebryn]

Looking good @machty. Do query param changes always use replaceState?

[asaf]

@machty Before merging please find a moment to look at:
http://discuss.emberjs.com/t/query-string-support-in-ember-router/1962/76

Or just http://jsbin.com/ucanam/2741# with some weird behaviors when having nested routes.

Thanks!

[machty]

@asaf Thanks! will do. There are a few more things, including this, that need some polish before it merges; I'll make sure to check in with you to make sure everything's good to go before merging.

[asaf]

@machty Cool, keep me updated when this issue is fixed so I can continue the migration,

Thanks.

[asaf]

@machty Issue I previously reported was fixed, thanks.

[machty]

@asaf nice! it did reveal some further issues so I'm still plugging away at some corner cases.

[asaf]

@machty Thanks, another thing, regarding TransitionToRoute behavior with QPs,

Assuming you have a route such admin.posts.index with corresponding controller with a QP 'someParam', invoking:

@transitionToRoute 'admin.posts.index', { queryParams: {'someParam': 'foo'} } won't work,

TransitionByIntend calculates query params changes by expecting QP to have full path of the controller name,

so old query param of a controller such 'admin.posts.index[someParam]' will be compared to 'someParam' which obviously will fail,

The user should provide the query param name only while behind the scene we should concat the fully qualified QP controller's name with it.

It's obvious that if you transition to some route you know ahead its name but:

1. Usability, it's ugly =)
2. It's 'half' internal (link-to, url calculation always does that automatically, I say half because we do expose the fully qualified QP in the URL as there's no choice)
3. Sometimes multiple controllers may provide the same QP and writing fully qualified query params makes the transitioning a bit more complex.
4. It may be a bit confusing, because routing to 'admin.posts' may actually route to 'admin.posts.index' while the QP expected to be FQ such 'admin.posts.index[someParam']

Thanks.

[machty]

"The user should provide the query param name only while behind the scene we should concat the fully qualified QP controller's name with it.

Yes that's exactly what should happen.

afaik the TODO list on this PR is:

1. Get active class working with link-to
2. The above behavior you just described

[asaf]

@machty 👍

You should carefully design the active property calculation of link-to, we discussed this a bit here:
http://discuss.emberjs.com/t/query-string-support-in-ember-router/1962/71

I can think of many reasons why to take into consideration QP when calculating whether a link is active or not and the opposite,

IMO we should support both, for instance we build on top of QP some data filtering mechanisms but the selected item is still the same even though QP may change, this behavior of course may not fit other scenarios.

[machty]

@asaf one thing that @alexspeller reminded me of is that you can always pass in a bound value for active within a link-to to override the default, e.g. link-to 'foo' active=someControllerComputedProp which gives you an opportunity to define some other easier logic than the full on complicated logic that is likely to be the link-to default.

But I don't know how ultimately sustainable that is; I don't think controllers should be containing logic about active class stuff; what we'll likely do in a later PR is add a subexpression helper for the active class to specifically to everyone's needs, e.g. link-to 'asd' active=(active-when 'asd' foo='123'), which could allow you to specify active class behavior separate from the params provided to the link-to helper.

[asaf]

@machty I dislike the active=controllerProp due to the reason you've mentioned,
But there are actually mainly two options:

Take QP into consideration and don't, mainly.

the 2nd option you suggested hard codes the values so i'm not sure how usable it is (unless u can say foo=currentFooValue somehow)

But yes, it requires some thinking, maybe consider waiting a little bit before implementing anything and see some community response.

[machty]

@asaf I've updated the PR to fix the transitionTo behavior as discussed, wanna give it a whirl?

[asaf]

@machty Will check this out,

Another issue I found, a bit hard to debug but I trust your work :)

Assuming you have some controller foo with query param and you hit that page,
then you change the query param which cause the queryParamsDidChange action to be invoked, then you callthis.refresh()` to perform a full transition cycle,

Now assume that the second hit cause the model hook to return a rejected promise, this bubble up to the application route's actions:error and we capture the failed foo transition and route to a login route,

Then in the login route we try to invoke transition.retry() on the capture route,

This should cause a transition to the foo captured route but it doesn't,
I think this falls somewhere in the new changes you've made in the transitionByIntent method, check it out!

1. Hit: http://jsbin.com/ucanam/2751#/foo?foo[something]=none
2. Change the query param something from none to something, will capture foo and route you to login
3. Push the login button which should perform a transition retry on the captured foo transition but it fails to.

Thanks a lot!

[machty]

@asaf The issue boils down to "what does it mean to retry a transition with a RefreshIntent, which is tied to a particular route hierarchy at the time refresh was called". Fortunately, there is a solution, hope to push the update tomorrow. Thanks for your continued gauntlet-testing

[asaf]

@machty Awesome.

[raytiley]

@gagoman try using canary. Query Params still isn't in beta channel yet.

[akhomchenko]

@raytiley same for canary.

ENV = {FEATURES: {'query-params-new': true}};

didn't helped too.

So there are no builds with this feature?

[Adriaaaaan]

It was removed from the beta channel unfortunately...

On 11 February 2014 20:15, Alex Khomchenko [email protected] wrote:

@raytiley https://github.com/raytiley same for canary.

ENV = {FEATURES: {'query-params-new': true}};

didn't helped too.

Reply to this email directly or view it on GitHubhttps://github.com//pull/4008#issuecomment-34800590
.

[raytiley]

@gagoman http://emberjs.jsbin.com/ucanam/3721/

[akhomchenko]

@raytiley thanks a lot. I've also noticed that I should to place ENV before ember script.

[rwjblue]

@gagoman Yes, since the the features must be enabled when Ember itself is evaluated, you must have set the EmberENV/ENV value first.

[skoryky]

Should the query params be available via controller properties in setupController? They don't seem to be, so I'm getting them via transition.queryParams, but then manually setting them on the controller before doing some controller setup. Seems roundabout, and I'm not sure if I'm missing something.

[joostdevries]

I'm curious about the answer to the questions by @ebryn and @mutewinter regarding using pushState/replaceState. At this point, no history entry is made for a query-params change which to me is unexpected behavior considering the fact that they usually modify the state in some way.

[machty]

@joostdevries we're changing the default to pushState and making it configurable in the next iteration, hopefully within the next 10 days

[joostdevries]

👍 Awesome!

[Globegitter]

Not sure if that is the place to post that, but just trying out this feature and it only seems to work in the ApplicationController for me and no other controller.
E.g.

App.ApplicationController = Ember.ArrayController.extend({
  queryParams: ['style'],
  style: 'test'
});

works, but

App.IndexController = Ember.ArrayController.extend({
  queryParams: ['style'],
  style: 'test'
});

does not and throws:

Error while loading route: TypeError: Object #<Object> has no method 'addArrayObserver'
    at Ember.ArrayProxy.Ember.Object.extend._setupArrangedContent (http://builds.emberjs.com/canary/ember.js:19698:23)
    at null._arrangedContentDidChange (http://builds.emberjs.com/canary/ember.js:19688:10)
    at sendEvent (http://builds.emberjs.com/canary/ember.js:2572:14)
    at notifyObservers (http://builds.emberjs.com/canary/ember.js:2958:5)
    at propertyDidChange (http://builds.emberjs.com/canary/ember.js:2803:3)
    at iterDeps (http://builds.emberjs.com/canary/ember.js:2841:7)
    at dependentKeysDidChange (http://builds.emberjs.com/canary/ember.js:2825:3)
    at Object.propertyDidChange (http://builds.emberjs.com/canary/ember.js:2801:3)
    at Object.set (http://builds.emberjs.com/canary/ember.js:3036:15)
    at ComputedPropertyPrototype.set (http://builds.emberjs.com/canary/ember.js:4992:13) 

Is that a known bug?

[raytiley]

@Globegitter can you provide a jsbin? Someone in #ember had this issue the other day and it was because they were using an ArrayController without implementing the model hook on there route.

[Globegitter]

@raytiley Oh yeah, if I provide the model hook, even with a simple return null it works. Will create jsbin in a second.
Edit: There we go: http://emberjs.jsbin.com/nafonute/1
Edit2: Yeah, if I use an ObjectController it works even without defining the model hook.

[Adriaaaaan]

just tried the latest canary and it gives me this error
<(subclass of Ember._MetamorphView):ember1561> Handlebars error: Could not find property 'query-params' on object Ember.Object:ember1555.

this is when its used from within an {{#each}}

[Globegitter]

This example here: http://emberjs.jsbin.com/ucanam/2849#/?foo[]=ray&foo[]=freakin&foo[]=tiley shows how queryParams work with arrays, which I got to work without any problems. But now my question, how can you use handlebars to have a link that uses arrays?

Trying to use brackets in the link-to handler just throws an error in the grunt-ember-templates task. Is that an error from the grunt-library or is that not implemented yet?
Here the example btw:
{{#link-to 'videos.index' (query-params cat[]=nameSlug) class='tag'}}{{nameCapital}}{{/link-to}}

[Globegitter]

I ran into another issue. I am using generator-ember as my build tool and running in dev-mode (non-minified and ember etc not concatenated) everything works fine. But as soon as I run it in server:dist mode, so everything is concatenated and minified I get the following error:

http://pastebin.com/tYQZtbqW

(It is really long, so thought I'd not pollute this thread here)

As a note, I have link-to with query-params in my videos.video template that link to videos.index as shown above. videos.index uses the query-params

@machty any idea what could be the issue?

[machty]

@Globegitter I'm guess something's rearranged in your dist build that is either removing your feature flag enablement code or moving it after the Ember library's been included.

[Globegitter]

@machty Oh yeah that was it. Thanks for the help. Also any word about the arrays in the link-to helper? Since you are already here ;)

[devendrawani]

Hi , I am playing with query-params-new feature, (just trying to migrate from Ember 1.4.0 Beta-Canary to 1.6.0 Beta-Canary) and came across this issue http://emberjs.jsbin.com/wulij

It looks like {{link-to}} helper is broken , it marking all links "active" irrespective of route.

Please to help.

[mutewinter]

I figured out how to force query params to make a history entry. Implement actions: {queryParamsDidChange:}, but don't call this.refresh. Instead, call this.router.router.refresh(this).method('updateURL').

Full example:

  actions:
    queryParamsDidChange: (queryParams) ->
      this.router.router.refresh(this).method('updateURL')

/cc @joostdevries

[devendrawani]

Hi guys, anyone looking into {{link-to}} helper issue for active routes ?Here is jsbin http://emberjs.jsbin.com/wulij

All links are getting marked as active irrespective of route. And it is happening with canary build only. :(

[panta82]

Hey, what's the status on this feature? I'm using it in my current project and really like it.

Is this still being developed / considered for inclusion? Or should I remove it while I still can do it relatively easily?

[devendrawani]

ivanpantic82 Its working fine in latest canary builds

[panta82]

Yeah, that's the point - it seems fine, yet it skipped 1.4, 1.5 and I don't see it in 1.6 beta so far. Is there some issue I should know about? I'm developing using canary build ATM, but I'd much rather move to beta or release before I get too far into production use.

[alexspeller]

https://speakerdeck.com/machty/emberconf-2014-mr-router-embraces-the-controller-alex-matchneer

[panta82]

Well that answers my question, thanks.
Seems QP-s aren't going anywhere soon, so I'll just stick with canary for another month or two (hopefully).

[szalishchuk]

What's the status on queryParams usage with retry()?
I use ember's production build 1.7.0 and get this error You didn't provide enough string/numeric parameters to satisfy all of the dynamic segments for route report, when I do retry on the transition to the route which has queryParams.

[ahacking]

@szalishchuk I had something very similar to your experience transitioning to another nested route using replaceWith from afterModel. I had to pass the current query params for the current transition to not get the same error as you. My stackoverflow "workaround" here:
http://stackoverflow.com/a/26480446/2238268

If you have a jsbin that replicates the issue then I am sure that would be helpful for someone like @machty who has intimate knowledge of the Ember router.

[szalishchuk]

@ahacking thanks I will try that out. Although since I do transition.retry() and not transitionTo/replaceWith I will have to remove a retry call and manually assemble transitionTo out of available transition object.