transloadit/rails-sdk

Options not working because to_json makes everything a string

michikono opened this issue · 14 comments

In Issue #3 you state you can pass arguments correctly. However, if I want to add a callback function such as :onResult => "my_function() { alert('do stuff') }", the resulting javascript ends up looking like:

"onResult": "my_function() { alert('do stuff') }"

onResult() is a string (rather than a function) and thus, not actually executed. It seems this happens because you parse the options using to_json, which would convert any Javascript code into a string rather than pass it straight through. Am I missing something here? Is there a better way to do this using your gem?

Unfortunately, I don't think there is right now. We want to let users pass a real Ruby hash to that method so they don't have to worry about the details of it emitting Javascript.

I would consider accepting a pull request that passes Strings straight through to the Javascript, but encodes everything else with to_json.

@stouset looked into it, shouldn't be too hard. Do you think about something like:

transloadit_jquerify :upload, '{wait: true}'

which would passed directly to JS while:

transloadit_jquerify :upload, :wait => true

would be encoded by to_json?

If so, I'll fix that tomorrow, push the commit and document it (off for today)

Alternatively, you could do it in Javascript. Perhaps something like:

VIEW FILE:

var transloadit_callbacks = { some: parameters }
transloadit_jquerify :upload, :wait => true

The JS that gets generated would then be refactored to merge in the parameters. As in:

$('#upload').transloadit($.extend(
  {
    wait: true,
  // merge the custom callbacks array into the ones the gem generated
  }, typeof(transloadit_callbacks) !== 'undefined' ? transloadit_callbacks : {}
)); 

*edited for jquery syntax correction

See here: #20

Honestly, that way seems a lot more obscure to me.

I can't think of a scenario where somebody would already be passing a String for that parameter, so it should be a very trivial patch to transloadit_jquerify. @rmehner has already volunteered, so I won't take away his thunder... :)

I'll try to keep the discussion on this thread. The problem with @rmehner's solution is that it means putting JavaScript code in a Ruby string, and then passed it in as a function argument that then gets parsed back into JavaScript. Not only is that difficult to debug if a developer typo-s something in the function, but it also forces the implementer to use pure JS. But these days, many Ruby developers are using CoffeeScript. My solution lets the caller decide what implementation language they wish to use and then to inject accordingly. It also lets the final code be more dynamic, since the page (i.e., browser) can alter the callback hash at runtime AFTER the your gem has already processed. This seemed more flexible to me.

Keep in mind that my problem is that I want to pass potentially complex JavaScript functions as callbacks to the transloadit jQuery API (you can attach callbacks to events such as an upload completing, I believe). As that logic gets heavier and more complex, the thought of passing that all in as a Ruby string scares me. It made sense to me that the solution kept the Ruby code light and maintainable (by the implementer). Unless I'm misunderstanding the solution presented?

I actually thought about it again and what I think now is the following: we know which callbacks are supported (currently everything that starts with "on" is a callback, see: http://transloadit.com/docs/configuring-the-jquery-plugin). So the patch I would write would do the following:

  • JSON encode everything that is not a known callback function
  • everything that is a callback is not encoded with JSON

If you guys accept that way I would write that and push it to this repo.

Tim accidentally closed this.

I like the solution, but it does mean we'll have to make sure the whitelist is always kept in sync with the potential callbacks. They're probably not likely to change, though.

Oops, sorry for closing this.

As for the callbacks: Yeah they won't change.

If there is a big update that would break bc, we'll announce it as a new jQuery SDK version anyway and keep the current one around for legacy.

@rmehner Still got this one? I can jump in if you're busy.

@stouset been sick over the weekend, patch will arrive tomorrow.

Pushed a fix. @michikono please test so @stouset can push out a new gem version.

@stouset: not too happy with the fact that I don't have any tests for this, but I'm on a train right now and internet connection is a bit limited. Couldn't figure out how to test the helper correctly without requiring abstract_unit from ActionView TestCases too. Maybe you have an idea :)

I did some tests within a test app and everything worked so far.

No worries. :)