JangoSteve / remotipart

Rails jQuery file uploads via standard Rails "remote: true" forms.
http://os.alfajango.com/remotipart
Other
1k stars 215 forks source link

Return Rails's standard value for `render` in `render_with_remotipart`. #191

Closed ordinaryzelig closed 6 years ago

ordinaryzelig commented 6 years ago

The alias method chain for render is not quite right. The return value of response_body here is not the same as render_without_remotipart here. Rails's standard render call returns a string while response_body returns an array. This PR makes it so that it does not alter Rails's standard behavior of returning a string.

I first noticed this in a project that relied on the return value of render which suddenly broke when I installed rails_admin, which includes this gem.

I started to test this, but ran into firefox binary installation issues and ran out of time. Hope you can forgive me for that.

mshibuya commented 6 years ago

This behavior is intentional, without it remotipart never work. If you have trouble with it, please open issue explaining the situation you're facing.

ordinaryzelig commented 6 years ago

This does not affect the main part of the code that makes remotipart work. The response body and content_type are still altered regardless of what is returned. However, because the method returns something different than rails, it causes any project that includes remotipart to behave differently than normal, even when a remote multi-part form is not being used.

There is, however a 3rd option that I think might satisfy both our cases. We could conditionally return a different value each time. E.g.

render_return_value = render_without_remotipart(*args)
if remotipart_submitted?
  # ...
  response_body
else
  render_return_value
end

When remotipart is submitted, then the return value is the same as before. But if it's false, then the standard rails return value would be returned and the render override will not affect standard behavior at all.

What do you think?

ordinaryzelig commented 6 years ago

To clarify my use case, I have a rails project that uses rails_admin, so ALL render return values are affected. It breaks my code that has nothing to do with multi-part remote form submissions. Ideally, I think when the gem is not needed (i.e. not remote form submission, not multi-part), it should not alter standard Rails behavior.

mshibuya commented 6 years ago

I understand your intention. Your '3rd' option might be preferable, since it preserves current behavior for remotipart requests. Could you update your PR accordingly?

ordinaryzelig commented 6 years ago

Updated.

mshibuya commented 6 years ago

Merged in, thanks!

JangoSteve commented 6 years ago

Thanks @mshibuya!

JangoSteve commented 6 years ago

And thanks @ordinaryzelig!