Removing URL params with undefined or null values

[jordanstephens]

I'm using this library all over the place in a very large application, and most of those places where I'm passing state objects as params to Route.*_path() functions. There are many cases where some of object properties are undefined or null at various times ending up with query strings which look like this:

?a=&b=&foo=bar

It would be really great if these functions automatically filtered out null or undefined values before building the query strings.

The nature of this library is that there are a ton of different, but very similar functions which end up getting called all over the place. Because of this, there isn't really a path to fix this kind of issue in my application code without writing code to filter these values in every single place we call one of these functions.

Surely the vast majority of users are not interested in params with empty values ending up in url query strings, especially, when you are working with many keys which may potentially be empty.

I will be happy to submit a pull request to have this fixed if you will merge it.

Thanks

[bogdan]

js-routes's serialization was made compatible with ones provided by jQuery and Rails.

jQuery.param({a: undefined, b: null, c: false}) // => "a=&b=&c=false"
{a: nil, b: false}.to_query # => "a=&b=false"

We don't aim for fixing word wide accepted spec here.

[bogdan]

The thing I can propose is to make serialize a part of the API so that you can redefine it. Wdyt?

[jordanstephens]

You are totally right that jQuery.param and ActiveSupport::CoreExtensions::Hash#to_query work this way, but I don't think that either of those are the correct model for comparison to the route helpers provided by js-routes.

It seems like one of the primary purposes of js-routes is to provide javascript route helpers with the same functionality as the "named route helpers" in Rails.

Let's assume we have the following Rails route definition:

get "/users/:id", to: "users#show", as: :user

Rails gives us this nice user_path helper:

user_path(1) # => "/users/1"

which corresponds nicely to the following Routes.user_path helper provided by js-routes:

Routes.user_path(1) // => "/users/1"

This is great! It's really cool to have route helpers that work the same way on both the server and the client side of my application.

Now let's take a look at what happens when I use the same helpers with parameters which may be nil or null:

Rails actually excludes keys with nil values from the query string:

user_path(1, { foo: "bar", baz: nil }) # => "/users/1?foo=bar"

while js-routes leaves the key in the string and gives it an empty value:

Routes.user_path(1, { foo: "bar", baz: null }) // => "/users/1?foo=bar&baz="

So, js-routes is inconsistent with the behavior of the named route helpers in rails, which I believe is one of the primary goals of js-routes.

Now, Rails does use ActiveSupport::CoreExtensions::Hash#to_query to construct the query strings, but it filters nil values from the Hash before doing so in ActionDispatch::Http::URL.add_params.

I noticed that you are actually using jQuery.param in the code if it is available, but if it is not available, you have some fallback code to use instead. Interestingly, this fallback code does not handle null values the same way as jQuery.param; it actually behaves in accordance with the rails code, by filtering out null values on line 48.

It would be really great if we could get the js-routes helpers to function in line with the rails helpers.

I propose simply removing the call to jQuery.param on lines 32-35.

[bogdan]

Yes, now I see your point. Thanks for explaining that rails url generation has some logic on top of to_query. I was thinking there is nothing there.

It is a little fail that jQuery presence changes behaviour. Especially with a fact that we have code that we have a test:

      expect(evaljs("Routes.inboxes_path({uri: null, key: undefined})")).to eq(routes.inboxes_path(:uri => nil, :key => nil))

That will fail when jQuery will be in place.
It should be fixed.

There is a reason why jQuery.param is there: there are too many edge cases to be supported in serialize. There are super crazy ones in jQuery. I don't remember the details but in 2012 it was so much easier to relay on jQuery for serialisation than fixing tons of edge cases.

So the solution

I propose to do 3 things:

1. make serialize function configurable:

JsRoutes.configure { |c| 
  c.serialize = "jQuery.param" 
  //or
  c.serialize = "function (params) { ... }"
  //or
  c.serialize = "MyAppNamespace.serialize_params"
}  

In this way people should be able to get previous behaviour any behaviour they want.

2. Make default serialize compatible with ActionDispatch's version : probably it means just remove jQuery.param call as you proposed.

3. put up a backward compatibility warning like Please specify serialize option in config to: nil if you want default and jQuery.param if you want jQuery compatible.

[le0pard]

I like number 1 idea - make configurable serializer.

[jordanstephens]

I totally understand the desire to use $.param to cover the corner cases, and that's fine. But before we consider making serialize configurable, how do we feel about just following rails by stripping null values before passing them to $.param? I worry that allowing a completely configurable serializer is a bit of an over-engineered solution.

I think users may want one of the following options:

1. $.param with null values
2. $.param without null values
3. some completely configurable thing

As you said earlier, serialization is not a super easy problem, so it's likely that any user-provided custom serializer will have a lot of bugs. I'm not sure that opening the door to a custom serializer is really a win for users, since they are most likely to want one of the other two options above. (I really think users will most likely want option 2, but I'm clearly a little biased, as it's also what I want).

[bogdan]

Implementation of striping nulls before calling $.param is a recursive algorithm that can have edge cases and bugs by itself.

Configurable serializer can be handy when you are using other Ajax library than jQuery. I am 100% each library that provides ajax has its own serialize. I am not sure why it wasn't asked before to support others than jQuery. I dont' believe people should implement it from scratch: not more than a little custom code on default options for existing one.

[jordanstephens]

Okay, a configurable serializer definitely makes more sense when you want to avoid maintaining the serialization code, and want to support multiple ajax libs. But if we do pursue that option, I have one more question: how would we allow users to configure it? The custom serializer would naturally need to be written in javascript, but all of the js-routes config code is written in ruby. This smells a little bit to me, but that's probably the simplest thing to do.

Something like this?

JsRoutes.setup do |config|
  config.serializer = "myApp.myCustomSerializerFn"
end

And I suppose we would just default it to jQuery.param

[bogdan]

Yes, configuration looks right. We don't expect people to write something like:

config.serializer = "function (params) { /* a lot of code here */ }"

Only newbie developer would do it and this option is only for pro.
Default to jQuery.param looks bad by 2 reasons:

• it is not compatible with rails url helpers
• it may not work out of the box for everyone: some people don't have jQuery

So I propose use our built-in by default that will be indicated as nil.

[jordanstephens]

Okay, that sounds good to me. I'll put together a pull request and send it over.