Initial QueryParams implementation

[alexspeller]

It's finally here. It works well. There are lots of tests. It makes sense. The API is as nice as possible whilst not introducing any backwards-incompatible changes. It works with all location implementations. There are nice, verbose error messages. It is, in general, super-awesome.

Live JSFiddle demo

Defining your query params

Query params are baked right into the routes. This is essential so that the router and helpers can know what valid transitions and query parameter states are.

App.Router.map(function() {
  this.resource('posts', {queryParams: ['sort', 'direction']}, function() {
    this.resource('post', {path: "/:id", queryParams: ['showDetails']});
  });
});

Model hooks

Note: Please see if #3383 has been merged yet to determine if the param order described here is still accurate

Query params are passed into all the relevant model hooks now. Only query params that have been registered to apply to the route in the router.map call will be passed in. If no query params are active for this route,
an empty object {} is passed in.

App.IndexRoute = Ember.Route.extend({
    beforeModel:      function( transition, queryParams ) {},
    model:            function( params, transition, queryParams ) {},
    afterModel:       function( resolvedModel, transition, queryParams ) {},
    setupController:  function( controller, context, queryParams ) {},
    renderTemplate:   function( controller, context, queryParams ) {}
});

Transitioning Query Params

transitionTo now will accept a final argument, which must be an object with the key queryParams.

this.transitionTo('post', object, {queryParams: {showDetails: true}});
this.transitionTo('posts', {queryParams: {sort: 'title'}});
this.transitionTo({queryParams: {direction: 'asc'}});

You can also use add query params to URL transitions as you would expect:

this.transitionTo("/posts/1?sort=date&showDetails=true");

linkTo Helper

The linkTo helper supports queryParams as well.

{{#linkTo 'posts' direction=asc}}Sort{{/linkTo}}
{{#linkTo 'posts' directionBinding=otherDirection}}Sort{{/linkTo}}

The linkTo helper now takes into account query parameters when determining its "active" state, and will set the class appropriately. The active state is determined by working out if you clicked on the link, would the query params end up the same? You don't have to supply all of the current, active query params for this to be true.

Stickiness

By default, query params are "sticky". This means that if you are on a url like /posts?sort=name, and you executed transitionTo({queryParams: {direction: 'desc'}}) or clicked {{#linkTo 'posts' direction=desc}}, the resulting url will be /posts?sort=name&direction=desc. This is true whilst you are linking to a route in the currently active tree.

To clear query params, give a falsy value (but not undefined), e.g.
transitionTo({queryParams: {direction: null}}) or {{#linkTo 'posts' direction=false}}

It's also possible to clear all query params by passing false, e.g. transitionTo({queryParams: false}) or {{#linkTo 'posts' queryParams=false}}

Boolean Query params

Boolean query params are serialised without the truth value, e.g. transitionTo('posts', {queryParams: {sort: true}}) would result in the url /posts?sort

TODO

Query param changes always hit the model hook. This is not always necessary. There should be a way to specify that query params don't affect the model.
Slightly more automatic way to observe controller params and serialise to url

Microlib PRs

tildeio/router.js#47
tildeio/route-recognizer#9

[alexspeller]

Grr the build is failing due to some updates in master since I branched. I will look at these failures shortly.

[sorentwo]

I've been following the query params discussion for a little while now. I'm really pleased with where you took it, it seems like a good blend of all the proposed methodologies. This will be very useful for me in the next week. Thanks @alexspeller 👍

[alexspeller]

Tests are now fixed :)

[wagenet]

This sounds awesome. We're about to make a stable branch for 1.0 so it might make sense to merge this in after we do that. Then it can come out as part of 1.1.

[dustMason]

👍 this looks fantastic

[Emerson]

How soon will 1.1 come out? I'm dying for this pull request to get merged! Thanks a bunch @alexspeller - this is really really nice to see in Ember core and would have saved me days of work on my own app.

[alexspeller]

@Emerson I imagine 1.1 is quite a way off, given the amount of stuff in it - however if all goes well this should be on master before too long

[alexspeller]

I implemented the ENV.NEW_MODEL_HOOKS_PARAM_ORDER flag and updated the PR.

[wagenet]

1.1 will be 6 to 12 weeks after 1.0. We'll have a beta out of it before release. We have a new scheduled release plan in the works that we'll be announcing soon.

[joshsmith]

@wagenet When is the new release plan going to be announced? More importantly: where?

[wagenet]

It will be announced on the blog. Most likely after the 1.0 release.

[ElteHupkes]

Woah Alex this is awesome! I'm going to have a deeper look soon when I have more time, but it seems really complete.

I do see a potential problem with linkTo; personally I use it's options often to pass options to the LinkView (a custom activeBinding is a good example). With this syntax it is impossible to differentiate between query parameters and view options, so I'm thinking there should be some kind option for that. I can think of two sensible ways to do this, the simplest yet ugliest being a postfix (or prefix):

{{#linkTo "some.route" viewOption="option" paramQuery="param" param2Query="param2"}}Link{{/linkTo}

Alternatively, a parameter specifying which options should be considered query parameters:

{{#linkTo "some.route" viewOption="option" param="param" param2="param2" queryParameters="param,param2"}}Link{{/linkTo}}

The latter can have a default of "if not specified use everything" or "if not specified use nothing". I think I prefer this approach, let me know what you think.

Looking forward to using this in an actual Ember build :).

[alexspeller]

@ElteHupkes I did consider this, but ultimately I decided that it's "not really a problem"™, here's why:

Because queryParams are so deeply integrated with the route recognizer, it's possible at link time to determine exactly which query params can apply to a linkTo.

For example, for this routeset:

App.Router.map ->
  @resource 'parent', queryParams: ['parentParam'], ->
    @resource 'child', queryParams: ['childParam']

Then only the following arguments to the linkTo helper are treated as query params:

{{#linkTo "parent" parentParam="someValue"}}
{{#linkTo "child" parentParam="someValue" childParam="someOtherValue"}}

This does mean that you can't name a query param the same as a property or method on Ember.LinkView, however in practice I'm not sure that this will be a big issue. Perhaps it is, I'm open to changing it if it's decided this is a blocker. In that case, a naming convention like:

{{#linkTo "parent" parentParamQuery="someValue"}}

is the only viable solution I can see. The other alternative you suggested still won't solve the clobbering problem.

Additionally, because the valid query params are known by LinkView, it's possible to warn when you use a query parameter in the linkTo helper that would clobber a builtin property, I don't know if this is a good idea or not.

[ElteHupkes]

@alexspeller Ah yes I totally forgot the fact that LinkView actually knows which fields can be parameters. That kind'a settles it the way the second option does, only more conveniently. I withdraw my argument 😉.

[ChristopheBelpaire]

Awsome feature, please merge ;) Heavily needed for simple stuff as pagination

[stefanpenner]

@ChristopheBelpaire this will in all likely-hood be part of the next release, and likely be available in canary/beta builds much sooner.

[knownasilya]

@ChristopheBelpaire Also a necessity for any map-based applications, due to things like Latitude/Longitude, Bounding boxes, and any filtering/sorting.

@ChristopheBelpaire this will in all likely-hood be part of the next release, and likely be available in canary/beta builds much sooner.

Glad to hear that, @stefanpenner .

[wagenet]

@alexspeller, can we get flags for this as described in the new release process post? @wycats, any objection to merging this with flags?

[alexspeller]

@wagenet I'd be concerned about doing that for a few reasons.

Firstly, these changes are largely in the microlibs so there will have to be duplicated code all over.

Secondly some things are really painful to feature flag without massive code duplication e.g. which dependant keys a computed property has.

Thirdly, and this is the core issue, doing this renders the test suite largely useless, because either (a) you have the feature flag on for all tests, which means you're not testing the behaviour when the flag is off (b) you have the feature flag on only for the query params tests, which means you're not testing that the rest of the system works correctly when the flag is enabled (i.e. does enabling the flag break non-query-params link-to?) or (c) you run all the tests with the flag on and off, which leads to a combinatorial explosion of test runs.

If I were to suggest "hey, I know you're not sure about adding this feature, so how would the core team feel about forking ember, and distributing and maintaining ember-with-query-params as a seperate project in parallel" you'd probably think I was insane, yet that's what having feature flags switching non-local, non-trivial changes actually gets you. In the blog post the benefit is touted as:

This will make it very easy for the core team and the community to see which features are in development and how far along they are.

This seems like a very small benefit to get from such a large amount of pain. You now have two ways to know what's in development, i.e. branches and feature flags. So now instead of having a stable, beta and master version, you have a massive amount of possible configurations depending on which flags are set, and it's pretty much guaranteed that each possible configuration has not been unit tested unless you set up travis to do (number of feature flags squared) builds, for each commit, before even taking into account the cross browser builds that are now happening, and then the non-extended-prototypes builds that already double the amount of tests to run.[1]

Frankly I think that although feature flags are a nice idea in theory, in practice they are a fundamentally flawed way to manage software release cycles. If the feature is not ready, it should not be included. If it is, then it should be included. Git branches are a very good, simple and functional solution that solves this problem very well. Feature flags are not.

[1] Note I think the non extended prototypes flag is the right approach - it's not a feature flag in the sense that it's a prerelease feature, it's something that's needed indefinitely and doing two builds is the right way to solve this. Adding combinatorial explosions of builds for temporary prerelease features seems crazy.

[wagenet]

@alexspeller I'm on my phone so I can't give a full response. Since much of the work here is in external libs, this may be an exceptional case. However, feature flagging is very important going forwards. It enables us to merge new features even if we are not convinced that they're production ready. The community can then help us vet the features and we have an easy means to remove them if they prove to be unstable. Features should be isolated and will be turned on for testing so that should not be a concern.

[wagenet]

While I very much want these features, I have some concerns about requiring a global flag for this. If we want this to be a first class feature, it should not be flagged. However, we cannot break apps using the existing API. Can you give this some further consideration. I really want this to be in 1.1 but we'll be branching soon so if this is not resolved soon, we'll have to wait for 1.2.

[wagenet]

I guess I should read more before commenting. This looks like it's really only a convenience issue. I am still a bit concerned about having this flag since it will fork code examples.

[alexspeller]

It is a convenience issue, the api isn't even horrible without it - but it's a tiny bit better with this turned on and long term I think it's a better API.

I don't really think it will fork code examples because I don't think many code examples are actually using the transition object passed into hooks, it's quite recent. I may be wrong about this though.

[wagenet]

@alexspeller is there a PR for this on the router.js repo?

[alexspeller]

I would like to hear your argument when you're not on your phone, as this is quite worrying to me:

@alexspeller I'm on my phone so I can't give a full response. Since much of the work here is in external libs, this may be an exceptional case. However, feature flagging is very important going forwards. It enables us to merge new features even if we are not convinced that they're production ready. The community can then help us vet the features and we have an easy means to remove them if they prove to be unstable.

Why is this easier than having stable / unstable branches? Why is removing them easier than git revert this way? If they are not production ready, they should not be in a production build. The community can help vet new features on unstable builds irregardless of feature flags.

Features should be isolated and will be turned on for testing so that should not be a concern.

This sounds really scary. If the flag is turned on for testing, then by default everyone using a production build without the flags on are effectively using completely untested code. Here is an example:

Ember.LinkView = Ember.View.extend({
  href: Em.computed(function() {
    if (Ember.FEATURES.isEnabled('queryParams') {
       // nice behaviour
    } else {
      throw "Oh noes";
   }
  }).property()
});

If you turn on the feature flags for tests, then the production build is now broken by default and the tests won't catch it. If you run two builds, with all feature flags on and all off, then what about the following code:

Ember.LinkView = Ember.View.extend({
  href: Em.computed(function() {
    if (Ember.FEATURES.isEnabled('queryParams') && !Ember.FEATURES.isEnabled('otherFeature'))  {
       // nice behaviour
    } else {
      throw "Oh noes";
   }
  }).property()
});

The production build is again broken and is not caught by the tests. Feature flags are a poor design choice because you cannot avoid this kind of problem without a combinatorial explosion of testing scenarios. Can you reassure me that there is a real solution to this problem, that doesn't involve running (n**2) test suites where n is the number of feature flags in the codebase? Because I honestly don't think there is a good solution and this is why I think feature flags are fundamentally flawed.

[alexspeller]

@alexspeller is there a PR for this on the router.js repo?

Yes, and route-recognizer, linked at the bottom of the description.

[wagenet]

Whoops, missed the links. Hard to do this on a phone screen :)

[wagenet]

@alexspeller trust me when I tell you that the feature flagging scenario is not as bad as you fear. For one, when features are turned off, they are not included in the build. Also, you are not allowed to use else or combine multiple flags. This reduces much of the complexity you are worried about. See http://emberjs.com/guides/contributing/adding-new-features/ for more information. Like I said earlier, it's possible we'll make exceptions in certain cases where the code is not easily flagged, though this means a more rigorous review process that may cause delays in merging.

[alexspeller]

This does reduce the complexity and having talked about in on IRC with you and @wycats I think that the likelihood of this actually causing breakages is relatively low (certainly to what's shipped as the default production build). I still hate feature flags but I can live with it :)

[wagenet]

To clarify, the code is in the beta branch, but it's turned off with a flag and completely removed from the beta builds.

[runspired]

@alexspeller is there usage documentation anywhere for this?

[alexspeller]

@jthoburn https://github.com/alexspeller/website/blob/3d3f3a18099ec93382595f87d40acab57bbb5cf2/source/guides/routing/query-params.md

[runspired]

@alexspeller Thanks!

[wildchild]

Awesome merge, just a couple of issues with params stickiness.

Assuming routes:

App.Router.map(function() {
  this.resource('movies', function() {
    this.route('index', { queryParams: ['channel', 'page'] });
    this.route('drafts', { queryParams: ['channel', 'page'] });
    this.route('edit', { path: '/:movie_id' });
  });
});

And a sidebar:

{{#linkTo "movies.index"}}Published{{/linkTo}}
{{#linkTo "movies.drafts"}}Drafts{{/linkTo}}

the page param is sticky, so if I am on /movies/index?page=5 and click Drafts router will transition to /movies/drafts?page=5. Should it stick here? Don't think so. This route is not nested to index.

Well, in order to avoid page stickiness sidebar goes to:

{{#linkTo "movies.index" page=false}}Movies{{/linkTo}}
{{#linkTo "movies.drafts" page=false}}Drafts{{/linkTo}}

Now if url is /movies/drafts?page=2, link in a sidebar is not active anymore. Remove page=false from linkTo and it will have an active class again regardless of query params.

@alexspeller should stickiness behave these way? Am I missing something?

[runspired]

This is something I felt could be improved on and had to hack a temporary solution to myself: I use an additional queryParam to specify the route taken, and then clear that param when transitioning into another route, using it to determine if the page param should also be cleared.

[alexspeller]

This is intended behaviour, although this specific use case is not
considered.

I think the problem here is that params with the same name represent the
same thing, however you are trying to use a single param to represent two
different things.

To solve your problem you can use two different params, e.g. moviesPage and
draftsPage.

[wildchild]

What is not specific then? Really a 2 not nested routes with page param that collide is so special case?

[alexspeller]

Query params are global. If you use the same name it is assumed to be the
same parameter. You have to use different names for different params at the
moment and there is not a simple solution to this because that's how urls
are structured. It is unlikely that this will change in the near future.

[runspired]

I've been sketching an idea for route specific name-spacing for queryParams in my head for the last week or so: essentially the idea is that when you declare valid queryParams for a route you can specify whether it should be global or not, and if not global then the given query params would be set to null when transitioning out of the route and before any queryParams were added for the route being transitioned into.

If I get anywhere with this I'll post a gist. Until then, the reason I didn't take @alexspeller's suggestion of just using different names but instead added an entire additional queryParam was because I needed #link-to highlighting to differentiate between a true & null version of the same queryParam, which unfortunately currently means a true or false version, but false params still register as being set within handlebars templates.

[adamlc]

I'm just wondering, I couldn't find this in the documentation. It seems like only the queryParams that have changed are passed in to the hooks? Rather than a copy of all the queryParams in the URL. I assume this is by design?

For me I generally pass the queryParams straight to the model to filter results. Meaning at the moment I have to have some logic to check to get / store the other params that haven't been passed.

Is there a better way of doing this?

[alexspeller]

@adamlc this is not correct. The query params that are registered for a particular route are passed in to the handlers, whether they have changed or not.

[adamlc]

@alexspeller in that case I must have something configured wrong! I think something must be wrong somewhere as I don't get the queryParams passed to the hooks directly, I have to get them from inside the transition variable.

I have:

Ember.FEATURES["query-params"] = true;

at the start of my code. Is anything else required? I'm using the latest canary releases.

[alexspeller]

@adamlc please open a new issue and provide a jsfiddle showing your code - if you tag me in the issue I will respond to it.

[alexspeller]

@adamlc but before doing so please ensure you have read and are following the instructions

[adamlc]

@alexspeller Just to let you know I went full noob, I was specifying the params on the resource and not the index route, doh!

[ElteHupkes]

I see this is enabled by default in 1.2, sweet! Great job Alex :).

I'm currently rewriting my code from using my plugin to these built-in methods, and I have a question: is it possible to update a query parameter in the URL without transitioning? My current use case is where I have a list with a "load more" button which loads an additional N (say 20) items from the API. I'd like to reflect this in the URL so that when the page is reloaded (or the URL is shared) the same number of items is loaded. However, when that button is pressed I don't load all the items, just the additional 20, I have custom logic for doing that. That means I would like to update the limit query parameter in the URL without transitioning. I suppose this isn't supported at this point (nor might it ever be), but would you have any thoughts on a workaround?

[alexspeller]

There is talk of the query params being completely rewritten, in which case they would support this. However I think a workaround currently has 2 parts:

1. Use replaceWith instead of transitionTo to avoid adding a state to the history
2. Add some code in the model hook, something like:

  model: function(params, queryParams) {
     if(modelIsLoadedAndOnlyLimitParamHasChanged) {
       loadMore();
    } else {
      loadFirst20();
    }
  } 

[rwjblue]

@ElteHupkes - Just wanted to clarify that this is NOT enabled on 1.2. See features.json for 1.2 which has it disabled.

[ElteHupkes]

@rjackson Ah it appears you're right.. I assumed it was enabled because I can see a lot of the code in there (just search the latest build for "queryParams") - but yeah without creating a canary build it's not enabled.

By the way, for who's interested, I ended up doing something similar to what @alexspeller suggested. In the route I have a loadMore action, which sets an instance variable on the route with the current model and some information about how many items need to be loaded. When model is then entered, it uses this information to load the additional items and return the existing list with these items in it (and unset the instance variable of course).

[tirana]

Will it be part of 1.3?

[rwjblue]

@tirana - No it will not be. The features that are enabled for 1.3.0 have already been set.

You can see the list in the features.json file on the 1.3.0-beta.1 branch.

[wagenet]

@ElteHupkes I think you're seeing the code from route-recognizer (https://github.com/tildeio/route-recognizer) that's vendored for use in Ember. That code isn't feature flagged at all.

[tim-evans]

this.transitionTo({ q: "fozzie", sort: "-last_name" }) doesn't work as of 1.3.0. _doTransition doesn't actually check whether only query params are being used for the transition.

[alexspeller]

@tim-evans are you definitely using a canary build and enabling the feature? Query params are not include in the 1.3.0 build. If you are then please open a new issue.