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

Repeated search parameters introduce unwanted polymorphism #1706

Closed
schmod opened this issue Jan 23, 2015 · 10 comments
Closed

Repeated search parameters introduce unwanted polymorphism #1706

schmod opened this issue Jan 23, 2015 · 10 comments
Labels

Comments

@schmod
Copy link

schmod commented Jan 23, 2015

Right now, if I want to pass multiple values into a search parameter, UI-router simply repeats the search parameter for every value that I pass in.

For example:

new UrlMatcher('/users?{id:int}').format({ id:[1,2] }); // '/users?id=1&id=2`

// In a state:
$stateParams.id; // [1,2]

However, if I only pass in one user ID:

new UrlMatcher('/users?{id:int}').format({ id:[1] }); // '/users?id=1`

// In a state:
$stateParams.id; // 1

This is less-than-ideal, because it makes $stateParams.id polymorphic, even when I've specified a typed parameter, and requires a lot of type-checking in controllers.'

At the very minimum, I end up with lots of:

var id = $stateParams.id;
if(!angular.isArray(id)){
  id = [id];
}
id.forEach(...)

Worse still, any state parameter risks being cast into an array if a user makes a typo, and specifies the same parameter twice. This seems likely to cause unpredictable breakages in controllers, given that most developers will not code defensively against this scenario.

IMO, the search parameter grouping introduced by #373 should require an explicit opt-in for each parameter (especially if the parameter is typed). If the matcher expects multiple values for a parameter, it should always deserialize the value(s) into an array, even if only one value is provided. If the matcher does not expect multiple values, it should return the first one that it finds, and discard subsequent matches (or throw an error).


Similarly, I'd also be interested to hear thoughts on introducing support for delimited search/query parameters (eg. /url?id=1,2). While UI-router's current implementation seems to be the most common, there does not appear to be a standard, and delimited lists arguably might produce more readable URLs...

Certainly not appropriate for all use-cases, but possibly quite useful...


Summary

  • Predictability is good.
  • Strong typing improves predictability. It's been an immensely useful addition to ui-router.
  • Type-checking is obnoxious, requires lots of boilerplate, and most developers won't do it if they're only expecting a single value.
  • If developers are expecting multiple values, they shouldn't need to handle a single value as a special case.
  • Defining a typed parameter should guarantee that the parameter's value always matches the specified type.
    • For the sake of simplicity and compatibility, the current behavior might be appropriate/acceptable for untyped parameters. However, it seems inappropriate for typed parameters.
  • Comma-delimited parameters might be cool?
@jonkoops
Copy link

Another popular way annotating that a paramater is repeated is by using brackets after the name of the paramater followed by the value.

For example: /url?id[]=1&id[]=2 would be deserialized into:

{
  id: [1, 2]
}

This annotation is used in the Ember router as well (http://emberjs.jsbin.com/ucanam/2849/edit)

@nateabele nateabele reopened this Apr 1, 2015
@lyschoening
Copy link

This seems to semi-work? /foo?{id[]:int} parses correctly even with a single item, but needs to be accessed as $stateParams['id[]'], which is tedious.

@georeith
Copy link

georeith commented Aug 7, 2015

Furthermore the value in the params object is ignored and the default value is always undefined when an empty array makes so much more sense, if I asked for an array type chances are I'm going to iterate it and when its default value is just an empty array I don't have to add checks to see if it exists.

This makes no sense to me.

@christopherthielen
Copy link
Contributor

Specify { array: true } in your param declaration docs (since 0.2.12)

array - {boolean=}: (default: false) If true, the param value will be treated as an array of values. If you specified a Type, the value will be treated as an array of the specified Type. Note: query parameter values default to a special "auto" mode.

For query parameters in "auto" mode, if multiple values for a single parameter are present in the URL (e.g.: /foo?bar=1&bar=2&bar=3) then the values are mapped to an array (e.g.: { foo: [ '1', '2', '3' ] }). However, if only one value is present (e.g.: /foo?bar=1) then the value is treated as single value (e.g.: { foo: '1' }).

params: {
param1: { array: true }
}

As for @georeith's point about undefined vs empty array, I think we made that choice to avoid a BC-BREAK. I agree though, if the param is explicitly specified an array type (either via array: true or via a name like id[]), it makes more sense to return an empty array than undefined.

@schmod
Copy link
Author

schmod commented Oct 23, 2015

I think we might be able to close this, given that { array: true } seems to document the current behavior, and offer a way to opt out of it.

@schmod schmod closed this as completed Oct 23, 2015
@juliancoleman
Copy link

juliancoleman commented Sep 21, 2016

what happens if you need the $stateParams to hold something of the nature of [["index", 10], ["offset", 0]]. What I get back is ["index,10", "offset,0"]

@christopherthielen
Copy link
Contributor

@juliancoleman $stateParams is an object, not an array. Can you explain what you mean?

@juliancoleman
Copy link

juliancoleman commented Sep 21, 2016

@christopherthielen I'm passing a series of queries to my db, and I record them in the URL for the user to bookmark. When the user visits this bookmark, it will execute a query based on $stateParams. So, my query will look something like { terms: [["index", 10], ["offset", 0]] }, which I'll go ahead and pass into $stateParams. However, when I revisit this page, my $stateParams will come back as a one-dimensional array, instead of the two-dimensional array. my terms now looks like { terms: ["index,10", "offset,0"] }, which will never execute the query because it's no longer in the proper format for my db to accept

@christopherthielen
Copy link
Contributor

christopherthielen commented Sep 21, 2016

State parameters don't automatically work with arbitrary data structures.

I think you have a few options:

  1. Use separate parameters index and offset

If you use a query string, this would be something like url: 'search?index&offset'
2) Use a single parameter and encode/decode it as json using the built in json type
this would be something like url: 'search?{terms:json}'

  1. Create a custom parameter type
    This is a little more involved, but allows you to encode/decode an arbitrary data structure as needed.

https://ui-router.github.io/docs/0.3.1/#/api/ui.router.util.$urlMatcherFactory#methods_type
https://ui-router.github.io/docs/latest/interfaces/params.paramtypedefinition.html

Here's a plunker with a custom param type that decodes to a series of arrays:
https://plnkr.co/edit/KueP1CaiDCqmwouTjHYy?p=preview

@juliancoleman
Copy link

@christopherthielen this is probably the most impressive answer I've received. I can't thank you enough for the time you spent to help me resolve this issue. That plunkr is exactly what I was looking for, but you also provided me with a means of declaring a JSON type as the URL parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants