emberjs / ember.js

Ember.js - A JavaScript framework for creating ambitious web applications
https://emberjs.com
MIT License
22.48k stars 4.21k forks source link

Add Query-String Support to the Router #1773

Closed wycats closed 11 years ago

caligo-mentis commented 11 years ago

Ok now we have two of these :) #1487 Maybe should close older one.

southpolesteve commented 11 years ago

It seems to me this also has some important implications for the view and the controller.

Controllers

It would be nice if the query string could be accessed in the controller or at least in setupController. Right now I am setting a controller property in the model function to support pagination:

App.ShopRoute = Ember.Route.extend({
  model: function(params) {
    this.controllerFor('shop').set('currentPage', params.page);
    return App.Item.find({
      "page": params.page || 1
    });
  }
});

This means I am stuffing what should be query parameters into dynamic segments. My URLs look like this: /shop/2/. It feels like a hack. setupController seems more appropriate, but that function does not have access to the params object. Here is the code I would like to write:

App.ShopRoute = Ember.Route.extend({
  model: function(params) {
    return App.Item.find({
      "page": params.page || 1
    });
  },
  setupController: function(controller, model, params) {
    this._super;
    return controller.set('currentPage', params.page);
  }
});

Really, it would be nice if it all just worked out of the box just like collections and individual records:

/users/ -> App.User.find() /users/1 -> App.User.find(1) /users?active=true -> App.User.find({active: true})

Views

The query string should be able to be set in the view, probably in the linkTo helper:

 {{ #linkTo 'shop' pageQuery='nextPage' }}Next Page{{ /linkTo }}

Assuming nextPage is a controller property and returns 2, the above would link to /shop?page=2. There are probably other solutions for setting the query string in linkTo and I'm open to suggestions.

Generally interested for feedback on both these ideas. I am not sure if my understanding of ember and javascript are up to the task of tackling these myself, but I would love to contribute any way I can.

caligo-mentis commented 11 years ago

@southpolesteve very useful comments. Currently, all of us use dynamic segments as params. I think we just need to extract query string from URL with router lib and make agreement about providing params to controllers. Maybe we should use separate method on Ember.Route like model one. Will wait decisions from @wycats and @tomdale.

jdjkelly commented 11 years ago

+1 to @southpolesteve

workmanw commented 11 years ago

@southpolesteve's comments are valid and provide excellent illustrations. For that reason, I'm closing my issue (#1487). Thanks @caligo-mentis.

seanrucker commented 11 years ago

I would like to be able to pass an auth token in the URL to automatically authenticate from an email and be immediately presented with the proper content. E.g. http://www.myapp.com/#/messages/5?auth_token=abc123. From what I can tell (still a newbie) this would be a pain (impossible?) to do with dynamic segments.

caligo-mentis commented 11 years ago

@seanrucker currently you can define the route with dynamic segments like that:

this.resource('message', { path: '/messages/:id/:token' });

and use model/serialize route methods to fetch model with this params.

seanrucker commented 11 years ago

Right but I would need to add that to every single route since I could potentially link a user to any page of the site from the email. Is there any way to do that globally?

caligo-mentis commented 11 years ago

Hmm, you can inherit all token-routes from custom abstract route which would just append token to params in serialize method and make manipulations with token in model method. But you'll need describe routes with custom path { path: /something/:id/:token' } anyway.

seanrucker commented 11 years ago

I'm getting around the issue by using the URL in the form of http://www.myapp.com/?auth_token=abc123/#/messages/5 and parsing the query string manually from window.location. Can you see any issues with this workaround?

alexspeller commented 11 years ago

I think the use case of global query params is important - the auth token use case is one scenario, however I'm dealing with global filters that can apply to lots of routes, e.g:

/foos?filters[type]=a
/bars?filters[type]=a

Essentially an ideal implementation will have flexibility when defining which routes accept and handle which query params.

alexspeller commented 11 years ago

Also implicit in my previous comment is that nested and array query params should be supported if possible, as in rails style

tylr commented 11 years ago

I too have a need for query parameter support. Right now I have several pages that need filtering.

To circumvent this limitation I store a queryParams object in the controller. The actions invoked from the view update the queryParams object accordingly. I then pass the queryParams object to the model find to update the content of the controller.

With this pattern working pretty well, I really wish I could just bind to the queryParams object and bind it to an event in the route.

alexspeller commented 11 years ago

This comment is out of date, please see https://github.com/alexspeller/ember-query

alexspeller commented 11 years ago

@southpolesteve @tylr @caligo-mentis @wycats any thoughts on this implementation?

alexspeller commented 11 years ago

This comment is out of date, please see https://github.com/alexspeller/ember-query

alexspeller commented 11 years ago

Gist has been update for ember rc1

alexspeller commented 11 years ago

Moved to https://github.com/alexspeller/ember-query

toranb commented 11 years ago

What is missing from the discussion / implementation to get this PR merged (aside from a good suite of tests) ? What else is holding up this issue?

alexspeller commented 11 years ago

Essentially everything ;)

This isn't even a pull request. Off the top of my head:

  1. It's an extension in coffeescript not a patch in javascript
  2. You are the only other person who has actually used it as far as I know
  3. There has been no discussion / feedback from anyone on the core team
  4. There has been no discussion / feedback from anyone except yourself
  5. It only supports history location and not hash location
  6. It has no support in the linkTo helper
  7. It has no tests

I am happy to work on the code issues / tests, however at the moment this is going nowhere without buy in from the ember core team. And as this has a milestone of 1.1 and they all seem quite busy, I doubt this is going to be given serious consideration for a while.

If I'm wrong about this, and they'd merge it if it was a proper pull request in javascript, with hash location support and tests, I'd be happy to work on those issues, but I just don't think it's a priority for the team at the moment, and I don't want to waste loads of time doing that right now to then find out that it's not what they want for query string support at all, and that I have a patch that constantly goes out of date against master to maintain until they get around to giving it the consideration it needs.

alexspeller commented 11 years ago

By the way, I'm glad that you're using it and it's working for you!

alexspeller commented 11 years ago

@toranb there is a new version, transitionToRouteWithParams was broken in some cases.

ElteHupkes commented 11 years ago

I've been taking a shot at implementing this in the core, after all Location based solutions (much like the one @alexspeller proposed, I use one for HashLocation), while functional in particular scenarios, are hacks at best.. There is one issue that keeps coming up: How do you determine which contexts are affected by the query string? When transitioning, route handlers higher-up maintain their context (which is a good thing). However, when the query string changes this could affect any of the active handlers; so without specific information about the routes you'd have to call deserialize on all of them. This is obviously not desirable behavior. The way I see it there are several approaches to this problem:

  1. Give every route handler a deserializeQuery method, which filters out the query parameters that the route uses; and save the return value of this method with the handler. On transitioning you can then compare the new query string object with the current one, and deserialize if there are any changes. I actually had this implemented at some point; but it felt like a hack..
  2. Only allow query string parameters on the "leaf" route handler.
  3. Allow a set of query string parameters for every handler; essentially allowing mildly strange URLs like /posts/index?sort=date:desc/comments?sort=date:asc. URL-wise this actually seems quite nice, as it contains query string parameters to the handler they affect. This would require support in the route-recognizer though, and so far I haven't been able to find a clean way to do add an optional ?=something to all Segments. (On a different note, why is route-recognizer so much more complex than the old RouteMatcher? Don't they pretty much do the same thing?).

By the way, all of this is assuming that the query string affects the model at all, which it does for my app (we paginate and sort server side, so query strings params are sent to the API), but I can imagine scenarios in which it influences something on the route/controller, but not the context; in which case you might only want to call setupControllers.

.. So that's my two cents. Hopefully I'm not ranting too much :). I'm still positive about implementing this sometime soon, but I'd very much like you guys' opinion on these things. I'll upload my implementation of 1. shortly to clarify what I'm talking about.

igorbernstein commented 11 years ago

I'm just passing through and haven't given it much thought to the repercussions in implementing this but what about having a syntax similar to Matrix Parameters?

/posts;sort=date:desc/comments;sort=date:asc or /posts;sort=date;sort_dir=desc/comments/sort=date;sort_dir=asc

ElteHupkes commented 11 years ago

Hmm I like that, it's like 3. but the syntax feels less awkward. Won't be any easier to implement though.

alexspeller commented 11 years ago

Hmm matrix parameters could work - it may even be the most elegant way to avoid calling deserialize on each route when transitioning - although, with the current solution I have implemented in ember-query, it is possible to affect the model without calling deserialise on every route. As the deserializeParams callback is called on every route, you'd just need to intercept this on the correct route:

MyPaginatedRoute = Em.Route.extend({
  deserializeParams: function(params, controller) {
    controller.set('content', MyModel.find({page: params.page});
  }
});

However this is a bit of a hack and I'm sure there are instances where this won't work. Thinking about it, the matrix params approach seems like the best compromise, especially as it allows global state to be maintained in parent routes - in fact, I suspect that matrix params would not even require any changes to ember.js or an extension at all - couldn't you just parse these out with the current route in the serialize and model callbacks that already exist, e.g:

parseMatrixParams = function(string) {
    params = {};
    var pairs = string.split(";");
    pairs.forEach(function(pair) {
      var kv = pair.split("=");
      params[kv[0]]= kv[1];
    });
  return params;
};

App.Router.map(function() {
  this.resource('posts', { path: '/posts/:params' });
});

PostsRoute = Em.Route.extend({
  model: function(context) {
    params = parseMatrixParams(context.params);
    return MyModel.find({page: params.page});
  }
});

I haven't tried this and it won't be robust at the moment ("/foos/bar=a-string-with-an-=-sign-and-a-;-in" for example) but is there any reason this won't just work at the moment?

ElteHupkes commented 11 years ago

The main issue with an approach like that is the parameters aren't optional; it forces the URI to end with a slash, although your posts URI should actually just be /posts when no parameters are specified. Also from having used something like this a while back I seem to recall there were some matching issues with just /posts/ without the parameters. And if you want something like /posts/:params/comments/:params there will be issues as well when the params are left empty.

kamal commented 11 years ago

This won't work if you expect the model hook to be called on PostsRoute. My solution was to host the dynamic segment on an inner index route. The PostIndexRoute will then be responsible for the serialize, so stash the params somewhere. When it bubbles up to the model hook of PostsRoute, simply call find with the stashed params. I stashed it in my Posts controller.

App.Router.map(function() {
  this.resource('posts', function() {
    this.route('index', { path: ':params' });
  });
});

PostsIndexRoute = Ember.Route.extend({
  serialize: function() {
    // parse and stash params
  }
});

PostsRoute = Ember.Route.extend({
  model: function() {
    return Post.find(stashedParams);
  }
});

You'll need latest master to get the PR that fixes this behavior. You'll also need to handle the case when the URL is accessed directly, where PostsIndexRoute#model will be called. I delegate to PostsRoute#model in this case.

toranb commented 11 years ago

@kamal could you show where / how you stash these params ? Just curious where these string values get put and pulled from dynamically

kamal commented 11 years ago

@toranb it gets stashed in the model. Here's a full example of how I have it set up. I spent an inordinate amount of time trying to figure out where to put the right callbacks and in what order they get called.

App.Router.map(function() {
  this.resource('posts', function() {
    this.route('index', { path: ':params' });
  });
});

PostsIndexRoute = Ember.Route.extend({
  model: function(params) {
    return this.container.lookup('route:posts').model(params);
  },

  serialize: function() {
    return this.controllerFor('posts').get('params'); // pull out from stash
  },

  setupController: function(controller, model) {
    if (model) {
      this.controllerFor('posts').setProperties({
        model: model,
        params: model.get('params') // pull out from stash
      });
    }
  }
});

PostsRoute = Ember.Route.extend({
  model: function(params) {
    params = params || this.controllerFor('posts').get('params');
    var records = Post.find(params);
    records.set('params', params); // stash in the model
    return records;
  }
});

If you go to /posts/foo from the URL, the flow is

  1. PostsRoute#model with no params value. Performs a find with params built from default state of the controller.
  2. PostsIndexRoute#model with foo in params. Calls PostsRoute#model again, but now with params.
  3. PostsRoute#model with foo in params. Do the find.
  4. PostsIndexRoute#setupController which stuffs PostsIndexRoute#model into PostsController (instead of PostsIndexController).

If you transitionTo into the route, the flow

  1. PostsRoute#model with no params. Performs a find with the current controller state.
  2. PostsIndexRoute#serialize. Serializes out from the same controller state.
kamal commented 11 years ago

I should mention that PostsRoute#model will not get called again if you transitionTo /posts/bar while in /posts/foo because it's already in the posts route. My hack is to transitionTo('postsTrampoline') where the route basically just redirects back to posts.

PostsTrampolineRoute = Ember.Route.extend({
  redirect: function() {
    this.transitionTo('posts');
  }
});
toranb commented 11 years ago

@kamal thanks for the quick reply -I might wait until RC2 is official to deploy this but it's a nice working example that helps me grok the router as it exists today! Thanks for the full example!

alexspeller commented 11 years ago

I had another idea about the deserialize stuff - perhaps a route could specify exactly which params affect it, e.g.:

PostsRoute = Ember.Route.extend({
   modelDependsOnParams: ['page', 'filters.type'],
   model: function(context, params) {
       return Posts.find({page: params.page, type: params.filters.type});
   }
})

It would require a slightly more complex implementation, but would result in the deserialize and model hooks being called on parent routes only when those query params have changed, and would therefore be more efficient whilst sticking with a more conventional querystring style. Every setupController hook in the state tree is called on a transition anyway so that's not an issue. @ElteHupkes what do you think of this approach?

ElteHupkes commented 11 years ago

Yeah my in-development app uses something like that (we seem to have the same train of thought), but it still feels like a hack don't you agree? This requires more configuration for the user where you'd want it to just work.

I'm currently trying to incorporate matrix parameters into the route-recognizer, I believe that would make things much more flexible.

alexspeller commented 11 years ago

I suppose "just working" would be better - I'm slightly torn because matrix parameters are strange and new but query parameters are comfortable and reassuring. I just happen to have never heard of them before so I'm trying to get my head around them.

To be honest, with the way the ember route works I think they make more sense conceptually, I think I just need to get used to the idea!

ElteHupkes commented 11 years ago

Alright I built something I'd like some opinions on.. Code is on this branch: https://github.com/ElteHupkes/ember.js/tree/query_recognizer, I created a quick JSFiddle to mess around with it here: http://jsfiddle.net/ElteHupkes/NCCKw/1/ (without the code but with address bar, which is quite useful in this case: http://fiddle.jshell.net/ElteHupkes/NCCKw/1/show/#/;msg=Hi).

Overview

this.transitionTo('posts', App.Router.routeQuery('posts', {'sort': 'date'}));
{{#linkTo "posts" sortQuery="date" directionQuery="asc"}}Static sort{{/linkTo}}
{{#linkTo "posts" sortQueryBinding="controller.sort"}}Binds query to the controller{{/linkTo}}
{{#linkTo "posts" queryBinding="controller.query"}}Binds the entire query object{{/linkTo}}

Known issues

What else

I had to make some pretty serious changes to RouteRecognizer for this, because its one-character state recognition would simply not support an optional query string in a path. I'm not exactly sure what the implications of that will be (or whether it'll make anybody angry :P); but it's probably worth mentioning that the entire test suite still passes after the change (actually there's two tests that fail; but these are unrelated tests that also fail on master).

I'm hoping for some feedback :)!

machty commented 11 years ago

@ElteHupkes great work! Haven't tried it out yet but seems good.

How do you transitionTo a route with both a context object and query params? I'm not 100% sure of use case, but if and when you need it, how would it be written in code?

ElteHupkes commented 11 years ago

@machty, thanks :). Transitioning with both a context and a query can be done like this:

this.transitionTo('posts', contextObject1, contextObject2, App.Router.routeQuery('posts', {'sort': 'date'}));

I modified transitionTo to separate the queries and context objects internally; it recognizes the type of object Router.routeQuery returns. So actually, above could be rewritten as:

this.transitionTo('posts', App.Router.routeQuery('posts', {'sort': 'date'}), contextObject1, contextObject2);

And it would do the same thing. The order of the query objects doesn't even matter, as they explicitly define which handler they're targeting. By the way, I'll update the code so routeQuery can be called from Route as well, then you can pretty much always use this.routeQuery (typeof less_code === "yay").

alexspeller commented 11 years ago

Hi all, FYI anyone using ember-query should update as I just pushed a bugfix that meant the docs were wrong and the browser back button often didn't work, that I was sure I already fixed ages ago.

machty commented 11 years ago

For the newcomers here (me and anyone else whose attention I'm directing to this thread), could @alexspeller and @ElteHupkes please highlight the different approaches taken to tackle this problem?

alexspeller commented 11 years ago

Sure - I'll summarize my approach used in ember-query. I'm going for a more traditional querystring based approach, which anyone familiar with rails will recognize. It uses jquery-deparam which is a rails-like queryparam deserializer. This will deserialize a querystring such as ?foo[bar]=1&foo[baz][]=2&foo[baz][]=3 into a javascript object like this:

{
  foo: {
   bar: 1,
   baz: [2, 3]
  }
}

This object will be passed into a deserializeParams hook on all routes that respond to that hook and are currently active. A corresponding serializeParams hook on all routes is called to create that object from controller state. A property called observeParams is added to the ControllerMixin, and any properties in that array are observed and trigger the serializeParams hooks on all current routes when changed, allowing you to bind controller properties to the current query string.

This approach gives a very standard, recognizable url format. However the disadvantage is that the same params are passed to all routes, i.e. they're all global and there's no explicit way to scope them to the routes they apply to, and identically named params clobber params on parent routes of the current state. This is not really a problem with a carefully chosen naming scheme. The real issue is that if your params affect the model for a parent route, the model hook will be called on that parent route even if the params change only should affect the child route. An example:

/posts/5/comments/page/1 - if you transition to /posts/5/comments/page/2 using the default router, the model hook for PostsController is not called again, as the router knows that hasn't changed. However using my library, /posts/5/comments?page=1 transitioning to /posts/5/comments?page=2 will need to call the model hook on both the posts route and the comments route, as the page param is global and there's no way to know which route it applies to.

The alternative approach allows params to be scoped to routes - the equivalent routes in @ElteHupkes solution, I believe, would be:

/posts/comments;page=1 and /posts/comments;page=2 - the page param being scoped to the comments controller. Although this looks similar but using a semicolon instead of a question mark, the difference is there can be params in parent routes, e.g. /posts;category=awesome/comments;page=2 - the category param applies only to the posts route but the page param only applies to the comments route.

I haven't tried his solution yet, and there may be a way to do this, but I don't think this solution supports global params - I currently am using filter params where the filters apply to both the parent and child routes, but I think this could be done by passing params from parent routes into child handlers.

TL;DR: My solution assumes params are always global, and urls are more traditional querystring-like. @ElteHupkes assumes params are always local to individual routes (I think), and uses a syntax that is not a traditional query-string syntax (but was proposed by Tim Berners-Lee in 1996) . Ultimately I think both are valid use cases, and a solution that allows both is necessary. Please correct me if I've misrepresented your solution in any way Elte!

machty commented 11 years ago

@alexspeller Great stuff, thank you.

ElteHupkes commented 11 years ago

Yeah, what @alexspeller writes is essentially right, parameters are scoped to the route handler and not global in my implementation. I chose that approach because of the parent-deserialization problem, and also because I think the syntax looks fancy ;).

Implementation-wise my solution is a patch to Ember's core that should work independent of your whether you're using HashLocation, HistoryLocation or WhateverLocation. @alexspeller's implementation is a custom Location (based on HistoryLocation) alongside some additions to Router. The former has the advantage that it could be merged to the core, but the latter can be used immediately without having to build Ember yourself.

Anyway I think that something like @alexspeller's solution has a bigger chance of being merged into the core (of course it'd have to be Location independent and submitted as a pull request); because it doesn't require the fundamental changes I made to RouteRecognizer, looks more familiar for most people and does the job 99% of the time (and some clever parameter naming will take care of the other 1%). However, the problem of unnecessary deserialization of parent routes would have to be solved. This would require detecting whether the query for any specific handler has actually changed after a transition, which is a pain because it sucks to compare JS-objects.

Anyhow, I'm building something new more which solves this problem, uses a classic query string and can be merged to the core as well as having a bolt-on version (it is of course powered by rainbows and will be endorsed by the unicorn community.. meaning I hope it'll work ;-]).

alexspeller commented 11 years ago

@ElteHupkes I had an idea - I suggested a modelDependsOnParams addition above but we agreed it should just work. How about this instead:

/posts/5/comments?author=alexspeller # author param affects all routes
/posts/5/comments?posts[author]=alexspeller # affects only posts route
/posts/5/comments?comments[author]=alexspeller # affects only comments route

Although that could be ambiguous, maybe more like:

/posts/5/comments?author=alexspeller # author param affects all routes
/posts/5/comments?_posts[author]=alexspeller # affects only posts route
/posts/5/comments?_comments[author]=alexspeller # affects only comments route

or

/posts/5/comments?author=alexspeller # author param affects all routes
/posts/5/comments?route.posts[author]=alexspeller # affects only posts route
/posts/5/comments?route.comments[author]=alexspeller # affects only comments route

i.e. some way of specifying in the param which route the param is scoped to?

ElteHupkes commented 11 years ago

Hmm that would work.. But it would expose implementation details (i.e. the name of the route handler) in the URL, which isn't very clean. I think there's no silver bullet here, so I guess what we should be going for is a solution which (a) works as expected, without configuration, in the most common situation. This "most common situation" I'd say would be a single resource route which allows something like pagination/ordering, so I guess the base case of what should "just work" would be /posts?sort=date with whatever parameters. A second requirement is that (b) it doesn't break more complicated scenarios without configuration (e.g. the unnecessary deserialization).

So the way I see it this we can either:

  1. Only allow query parameters on leaf routes
  2. Disable parameters by default, and allow them through some sort of modelDependsOnParams (actually I preferred observesParams or maybe the full observesParameters).

(1.) Has the obvious disadvantage that if you want parameters on non-leaf routes, you can't. I believe we both have cases where this restriction is violated already, so that pretty much makes it a no-go for me. (2.) Has the disadvantage that it doesn't really "just work", but rather doesn't "just fail". However, I highly doubt if we can achieve "just works" anyway, because this would also require some expected behavior from model. And even though to us it makes sense that PostsRoute.model({sort: 'date'}) should do Post.find({sort: 'date'}), the handler won't actually know it has to use Post for the find, as there is no "post_id" to derive it from as is the case with dynamic segments. "Singularizing" the routeName would work, but this is a whole different problem altogether (posts -> post is simple, but categories -> category is not). On the other hand, (2.) has the advantage that it is flexible, and allows any naming/linking scheme you want (and you're free to choose a naming scheme to scope your routes).

TL;DR, I think your initial approach might have been best. What do you think?

machty commented 11 years ago

fwiw, it's not a problem if the solution involved changes to the microlibs. If it can be argued that query-strings are something that a router recognizer should be able to handle, then the change belongs there. Yes, it's a little more complicated to coordinate the change, but don't let that sway you from the correct solution.

EDIT: apologies for the unproofread engrish

ElteHupkes commented 11 years ago

@machty Fair enough.. The thing is, I spent quite some time understanding how the recognizer works, and an almost equal amount of time wondering why it was designed that way. Especially since the old router contained a recognizer with - as far as I can see - the same functionality, but way way less complex (I believe that just looking at the code the difference is like 80 lines vs 500 lines). So I sort of figured there must be a very good reason for it; though I'd like to be wrong :). Anyway, a global query string can be split off in the router; but scoped parameters have to be incorporated into the recognizer.

machty commented 11 years ago

If I remember correctly there's more functionality in route recognizer for flexible routing, wildcard star routes, etc. If the old router seemed like less code it's probably because a lot of the functionality was already there, ish, blurred into some other mechanism, but I've come to appreciate the Unix-y do one thing and do it well approach that Ember's trying to take more recently with its microlibs.

I'm a fan of the per-route query-strings, I'll try to get some other people to weigh in.

machty commented 11 years ago

More to say on this: for a Rails router, global query strings make sense since a router ultimately leads to a single action on a single controller. This is where the overlapping terminology b/w Rails/Ember can be misleading, since to navigate to route in Ember, you have this building up of multiple controllers and Routes each presiding over a small chunk of the URL, rather than one takes all. For this reason I think it would be wise to opt for the less conventional querystring-per-route approach rather than picking something that looks familiar but ultimately would need to be split out to the controllers/routers that need that information.

While I'm in favor of the per-route-querystrings, is there some reason we couldn't have global query-string support as an enhancement to Ember core that would work for all Location types?

alexspeller commented 11 years ago

There's no reason the global approach couldn't work with all location types - it's just that I haven't needed to implement that yet. I think I am coming to prefer the per-route option as it does sit better with the design of the ember router.