jquery-ujs Add data-params handling to handleMethod

This commit adds the functionality to fetch the data-params attribute from the link and add fields to the form that express the values from the data-param attributes.

This is useful for POST-requests via links that should post parameters instead of sending them in the URL.

My version of forgotten #307 from @phillipp, with escaping and composing hidden inputs from complex objects by @bcm's request.

Oct 28 '14 10:10 KODerFunk

:+1: good feature!

Oct 28 '14 10:10 stereodenis

:+1: would love to see this merged soon ;-)

Oct 28 '14 10:10 phillipp

It seems like this is re-building the functionality of a <form> element using data-remote. button_to generates a <form> element, can be used with remote: true and has a :params option for specifying data with hidden inputs.

If it needs to look like a link, that can be achieved through CSS (see bootstrap's btn-link CSS class).

Oct 28 '14 14:10 ecbypi

Yes. This is doing the same as button_to helper. Could you explain why not using it?

Oct 28 '14 16:10 rafaelfranca

@ecbypi, functionality of a <form> element using data-remote for use data-params realized in handleRemote method. My implementation is reasonable compliant functionality of using data-params with data-method attributed elements without data-remote (non ajax). For example

<a href="/admin/parsings" data-method="post" data-params='{"kind_id": "1"}'>Start some kind parsing</a>"

Oct 29 '14 10:10 KODerFunk

I don't think we should promote JSON usage on data tags. If you need to pass params in a link use the button_to helper.

Oct 29 '14 17:10 rafaelfranca

@rafaelfranca and how to use button_to helper to pass params without data-remote ?

Oct 29 '14 17:10 stereodenis

It created hidden inputs inside the form tag.

Oct 29 '14 17:10 rafaelfranca

@rafaelfranca, do you think reasonable that parameters are passed in this case:

<a href="/admin/parsings" data-remote="true" data-method="post" data-params='{"kind_id": "1"}'>Start some kind parsing</a>"

but there is no way?

<a href="/admin/parsings" data-method="post" data-params='{"kind_id": "1"}'>Start some kind parsing</a>"

Both constructions are handled by jquery-ujs, but in the second case surprisingly no parameters are passed.

Oct 29 '14 18:10 KODerFunk

O_O. If first is working, there is no reason for the second to not work.

Oct 29 '14 18:10 rafaelfranca

I add test for first case, demonstrate coorrect work of JSON params. And now i work on removing unnecessary escapeHTML. I doubt the necessity of https://github.com/rails/jquery-ujs/commit/443de05134eebb27f5e18b43cb3ca0088b3a86b3. In latest FF and Chrome (MacOS) pass all test without escaping. Now will testing in old IE versions (Windows).

Oct 30 '14 10:10 KODerFunk

@KODerFunk :+1:

Oct 30 '14 14:10 stereodenis

@rafaelfranca what do you think about this PR?

Oct 31 '14 16:10 stereodenis

I prefer to defer this decision to @JangoSteve or @lucasmazza?

Oct 31 '14 17:10 rafaelfranca

Sorry, I had to go back and read through the conversation from #307 to remind myself of the initial intent and discussion.

There are a few issues with this PR as it stands, that lead me to believe that #307 might be closer to the desired implementation:

We shouldn't have the buildParamsInputs function defined the way it is within the handleMethod function. If we're going to implement that function, it should be extracted out so it can be reused.
That said, I don't necessarily like adding the buildParamsInputs function in the first place, even if it were to be extracted out. If we want to be able to parameterize arrays and hashes, we should use jQuery built-in functionality for that, the same way jQuery does it for $.ajax() functions when you pass the data attribute. For example, see $.param() and $('form').serialize() functions in jQuery. This would help us keep with the element of least surprise.
Why are we changing test.server.rb for this pull request? That's a low-level thing that's used by a lot of the tests in the test suite. I'd prefer it to remain unchanged, or if it's really needed for some reason, it should be done in a separate pull-request with an explanation that shows some understanding of why it was like that in the first place.

I say this, only because I honestly don't remember off the top of my head why it is the way it is, but I can tell you that a lot of thought and effort went into writing it the way it is, back when we did it; it was just a long time ago. Then again, I could be wrong, and maybe there's no good reason for it to be that way.

But it may have had something to do with getting the tests to pass in all browsers. Did you try running the updated test suite, for example, in Opera and older versions of IE? That's where a lot of the weird code like that in the test suite came from.
I don't fully understand the default handler in your case statement. I think one of the main use-cases of this would be my comment here data-params="my_value=hello", like you can with remote links and other remote elements.

As far as I can tell, your implementation does not allow any sort of setting params using the HTML data-params="..." attribute, because if you have an element with this, this is going to pass null in as key, and your string in as the value, which will then produce a hidden field with name="null" and <input name="null" value="my_value=hello&other_value=bye"" type="hidden" />, which isn't going to do what you expect it to.

Of course, as the test cases show, this implementation only supports directly setting $("#my-link").data("params", {some: "object or array"}) programmatically via JS.