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

Add Query-String Support to the Router #1773

Closed
wycats opened this issue Jan 14, 2013 · 79 comments
Closed

Add Query-String Support to the Router #1773

wycats opened this issue Jan 14, 2013 · 79 comments
Assignees

Comments

@wycats
Copy link
Member

wycats commented Jan 14, 2013

No description provided.

@caligo-mentis
Copy link
Contributor

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

@southpolesteve
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

+1 to @southpolesteve

@workmanw
Copy link

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

@seanrucker
Copy link

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
Copy link
Contributor

@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
Copy link

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

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

@tylr
Copy link

tylr commented Feb 4, 2013

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
Copy link
Contributor

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

@alexspeller
Copy link
Contributor

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

@alexspeller
Copy link
Contributor

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

@alexspeller
Copy link
Contributor

Gist has been update for ember rc1

@alexspeller
Copy link
Contributor

@toranb
Copy link
Contributor

toranb commented Mar 18, 2013

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
Copy link
Contributor

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
Copy link
Contributor

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

@alexspeller
Copy link
Contributor

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

@ElteHupkes
Copy link

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
Copy link
Contributor

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
Copy link

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

@alexspeller
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

kamal commented Mar 29, 2013

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
Copy link
Contributor

toranb commented Mar 29, 2013

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

@machty
Copy link
Contributor

machty commented Apr 5, 2013

@igorbernstein Too much cake.

I can't really think of a compelling use case for this that couldn't be handled by a leafier route looking up information on the parent route that was passed a per-route query string. It shouldn't be ambiguous which route will be using the author= information. Forcing per-route query strings would remove that ambiguity.

edit: 🍰 🍰 🍰

@ElteHupkes
Copy link

Hmm also not sure it's desirable. I can probably think of a use case for it, but then you'd have to explain to everyone why there's two types of query strings, which takes preference and when and how they're updated.. We'd spend all day in the kitchen only to see our guests stare in doubt at our cake, which is an octahedron with whipped cream on all sides.

@machty
Copy link
Contributor

machty commented Apr 5, 2013

Cake Wreck

@Nthalk
Copy link

Nthalk commented Apr 17, 2013

I've got a decent workaround by create an outer controller that has no view or templates:

/todos/p/key:value,key:value
Application => Todos => TodosParams

It works with the controller context stack and reaching up in the stack to pull arguments from a params controller that sits below the desired controller's route.

When you need parameters, you need the params controller, and set a paramsBinding: "controllers.params"

A rough example how to do it manually is here:
https://gist.github.com/Nthalk/5407240

A more involved helper function exists here:
https://gist.github.com/Nthalk/5409344

Edit: This method doesn't work well, see my below comment for my revised strategy

@alpacaaa
Copy link

@Nthalk thanks for sharing, simple and useful!

@toranb
Copy link
Contributor

toranb commented Apr 18, 2013

Agreed! @Nthalk could you post a full jsfiddle with a sample app using this? I'm trying to use a model route and found that having "params" be a string that represents multiple model properties a bit awkward so I'm sure I'm just doing it wrong :)

@Nthalk
Copy link

Nthalk commented Apr 18, 2013

Okay, after diving into that for a while, I realized my above solution didn't work that well, and there was a different, more usable way of dealing with it:

http://jsfiddle.net/fdkPY/7/
NOTE: jsfiddle isn't showing the underlying routes in the address bar. They're being hidden in some iframe, but they do reflect dynamic parameters.

The one major caveat of this method is that it crowds the url space. Whereas you once had:

todos              # todos.index
todos/:todo_id # todos.show

...You cannot use that uri space because it is taken by :params, so you must instead use something like...

todos/list/:params # todos.index
todos/:todo_id      # todos.show

...so you don't collide with the resource routes. It's a small price to pay, but you can see from the jsfiddle that this method does indeed work and allows extensions into the show routes:

todos/:todo_id/:params      # todos.show withParams

In my projects lib, I'm planning on adding a WithParams mixin that does the generic parameters lifting.

Additionally, there can be an observer that listens to changes on the params and updates the current URI via transitionToRoute, but that's best left to the developer to decide if calling transitionToRoute on an observer is sane.

@jayphelps
Copy link
Contributor

This is super required for me to keep state within the url. Example: /session?tool=draw&theme=something&redirect_uri=something

Anyone taking a crack at this yet for a pull request?

@alexspeller
Copy link
Contributor

@jayphelps have you tried my library?

@jayphelps
Copy link
Contributor

@alexspeller I have indeed, but I require history and hash support both (I'm currently using my PR #2685 for AutoLocation). I'd be down adding that support to your library, but if this is indeed something the core team wants in the mainline I'm hoping we can get a PR rolling.

@alexspeller
Copy link
Contributor

Ah ok. Well the problem is that my implementation is the most complete, however it's unlikely to be the officially adopted one. I haven't worked on matrix params at all, and am unlikely to in the very near future, although I'd be willing to at some point when I have more time.

@ElteHupkes
Copy link

Guys! I've been working on another query parameter workaround lately, and this one I'm pretty excited about. It can be found here: https://github.com/ElteHupkes/ember-query-params . Quick summary: global query string (not my preference, but less impossible to implement), works with linkTo / URL generation, Location independent, minor route configuration. For me personally this is the best solution thusfar - let me know what you think!

@coderberry
Copy link

@ElteHupkes great contribution. Do you have an example app which demonstrates it's usage and benefits?

@ElteHupkes
Copy link

@cavneb I've cooked something up real quick: http://liveweave.com/QLJ0f1 . Unfortunately these JSFiddle type-things don't show the URL-bar, which really is kind of the point when showing query parameters ^^. You can copy-paste that LiveWeave's code into a HTML-document and it should work though.

Any questions, please let me know :)!

@coderberry
Copy link

@ElteHupkes it worked great. Here's an example app: https://github.com/cavneb/ember-query-params_example

@ElteHupkes
Copy link

@cavneb Nice! I added a link in the README.

@alexspeller
Copy link
Contributor

@ElteHupkes I've been a bit busy to update my libary for RC6 at the moment, but it seems our libraries are very similar, in fact when I do update my library I will probably be looking at yours and basing it mainly from your work as it does most of what I need. However I think there are one or two things my library does that yours doesn't (binding params to controller properties is the first one I can think of), and one thing I'm not too keen on with your library (I don't like the QueryParameters and don't think it's necessary).

In any case, I'd like to ask if you'd be willing to work together on this problem and find a way to combine our efforts?

@ElteHupkes
Copy link

@alexspeller Definitely! I'll probably be too busy the upcoming weeks for any serious meddling with the query params (ok part of that will be a vacation, so maybe "unavailable" is a better term ;]), but I think it's a very good idea to put our heads together and start thinking about how we can combine our efforts and make things better for everyone.

@alexspeller
Copy link
Contributor

Great! I will start working on some stuff, and I'll file some pull requests to your repo and put a note on my library to look there for RC6 support. Enjoy your holiday, and let's talk when you get back.

@coderberry
Copy link

@ElteHupkes @alexspeller God bless both of you for working together to solve this. I wanted to try ember-query but it's not RC6 ready yet.. I already have an app that's near production using ember-query-params, which I really like.. I will integrate your combined efforts as they become available. Thanks again!

@ghost ghost assigned machty Jul 19, 2013
@dustMason
Copy link

Thanks for taking this on @alexspeller and @ElteHupkes! I just grabbed ElteHupkes/ember-query-params and added it to a project I'm working on to give it a spin and wanted to chime in to agree with one of @alexspeller 's recent comments:

I find the QueryParameters object a little unwieldy, especially regarding the linkTo helper. I'd like to be able to specify my query params right in the template much like I do in my rails apps. I'm not experienced enough with Ember to know how/if this would be possible, but it would feel most natural to me.

@machty
Copy link
Contributor

machty commented Jul 30, 2013

Hi all,

This GH thread is becoming a Tolstoy novel, and I'd like to get some outside thoughts in since I doubt there's all that many people following this thread beyond some of the main contributors. So, if you will, please check out the following Discourse thread and summarize the various approaches you've taken, lessons learned, implementation status, etc., and see if we can't nail the best way to go going forward:

http://discuss.emberjs.com/t/query-string-support-in-ember-router/1962

@alexspeller
Copy link
Contributor

I just pushed support for ember master here

@MiladQamari
Copy link

hi alex

ember-query doesn't work in IE 9 , I added new issue with error message

@alexspeller
Copy link
Contributor

Hi all, there is now a PR for this, please see #3182

@wagenet
Copy link
Member

wagenet commented Sep 6, 2013

Closing in favor of discussion on the PR.

@wagenet wagenet closed this as completed Sep 6, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests