Use Module.prepend when possible

[chrisarcand]

WARNING: See this comment. Basically, this removes the deprecation on Rails 5 but the functionality is probably broken (for previous versions as well), anyway. On ManageIQ we just don't use it, so it doesn't matter really. The bottom line is that this gem is essentially a wasteland and nobody should honestly use it. The better solution is just to stop using RJS.

This removes current deprecation warnings with Rails 5 and will allow continued use of the gem once alias_method_chain is removed from ActiveSupport.

This change allows both patches in this gem to utilize Ruby 2's Module.prepend instead of ActiveSupport's alias_method_chain when possible. Normally I would drop support of Ruby < 2 and just rely on prepend, but as this gem is largely for legacy purposes this now allows for both methods depending on the version of Ruby used.

Testing / QA

Annoyingly, I have no idea how to test this. The gem's test suite is in such a disarray that I don't think they'll ever really pass at this point (before these changes), and using the RJS functionality we utilize on ManageIQ doesn't actually trigger the code patched here. So... yeah. Careful code review I guess.

[chrisarcand]

For ManageIQ/manageiq#8996

[djberg96]

👍

[NickLaMuro]

Comments on implementation.

[NickLaMuro]

From a (arguably pedantic) performance perspective, I would like to see this conditional not in the method definition, but wrapped around defining the method. This would add more code, but doesn't include a conditional on every invocation of this method that will be the same outcome every time.

[chrisarcand]

So... you'd rather see different methods entirely with one RUBY_VERSION check than a meta'd single method with multiple version checks?

if RUBY_VERSION < 2 alias_method_chain with 'render_with_update', else prepend 'render', a separately defined method ?

[NickLaMuro]

Basically. Again, arguably pedantic, but this is technically changing the existing functionality, when the whole point of this PR as I see it is to retain the same functionality sans deprecation warnings.

What I am suggesting is "on load, if ruby 2, define this method, otherwise this method", instead of "when this method is called, are we still ruby 2? okay, then execute the same block of code again". With the second one, unless you can pull off swapping out your ruby runtime mid process (which would be incredibly impressive, and also, why?), then there is no point in that check existing in the method, and it should just determine how it is defined.

That said, saying this is a "performance concern" is probably laughable, but it seems silly to include an in a method when it is an code load evaluation concern, not a method runtime concern. Silly, but at the same time, not worth investing if there isn't a easy solution (mine poor solution was written not knowing how alias_method_chain functions in the first place...)

[chrisarcand]

but this is technically changing the existing functionality

I don't see that. Where? (Note I had to fix some code with your first comment, should be the same semantics now though)

What I am suggesting is "on load, if ruby 2, define this method, otherwise this method", instead of "when this method is called, are we still ruby 2?

That's fine, it just duplicates logic for the method. The way to do this is to just have two different modules and prepend/include and alias one depending on the ruby version.

module LegacyRenderingHelper
end

module RenderingHelper
end

if RUBY_VERSION < '2'
  # eval the Rails module, include the LegacyRenderingHelper, alias_method_chain
else
  # prepend the normal RenderingHelper
end

The issue with this is mostly the fact that I would have to either duplicate the entire module of helper methods (in the other patch in this PR) or do some Ruby module mincing of a third module before doing prepend/alias_method_chain. So...kind of a tossup. Thoughts on that?

[chrisarcand]

The issue with this is mostly the fact that I would have to either duplicate the entire module of helper methods

selector_assertions.rb, if that wasn't clear.

[NickLaMuro]

but this is technically changing the existing functionality

I don't see that. Where? (Note I had to fix some code with your first comment, should be the same semantics now though)

Sorry, that was unclear. From the perspective of input to output, they should be the same. I am more referring from evaluation stand point in which they differ (what if statements and other method calls they have to traverse to get said output, etc.). I would explain further, but I think you get what I am saying (I hope). Regardless, I was unclear... my bad.

The issue with this is mostly the fact that I would have to either duplicate the entire module of helper methods (in the other patch in this PR) or do some Ruby module mincing of a third module before doing prepend/alias_method_chain. So...kind of a tossup. Thoughts on that?

I think it depends on if you buy into the B.S. argument I have been making this whole time 😉. That, and if it is worth the effort to do it this way. That all said, isn't the only change that your code for the selector_assertions.rb be:

module JqueryRjs::SelectorAssertions
  if RUBY_VERSION < '2'
    def response_from_page_with_rjs
      # ...
    end
  else
    def response_from_page
      # ...
    end
  end
end

module_to_patch = if defined?(ActionDispatch::Assertions::SelectorAssertions)
                    ActionDispatch::Assertions::SelectorAssertions
                  else
                    Rails::Dom::Testing::Assertions::SelectorAssertions
                  end

if RUBY_VERSION < '2'
  module_to_patch.module_eval do
    include JqueryRjs::SelectorAssertions
    alias_method_chain :response_from_page, :rjs
  end
else
  module_to_patch.prepend(JqueryRjs::SelectorAssertions)
end

Arguable, you could split out the code in response_from_page_with_rjs to be something like this to avoid duplication:

def __response_from_page
  content_type = @response.content_type

   if content_type && Mime[:js] =~ content_type
    body = @response.body.dup
    root = HTML::Node.new(nil)

    while true
      next if body.sub!(RJS_STATEMENTS[:any]) do |match|
        html = unescape_rjs(match)
        matches = HTML::Document.new(html).root.children.select { |n| n.tag? }
        root.children.concat matches
        ""
      end
      break
    end

    root
  else
    yield
  end
end

if RUBY_VERSION < '2'
  def response_from_page_with_rjs
    __response_from_page { response_from_page_without_rjs }
  end
else
  def response_from_page
    __response_from_page { super() }
  end
end

But not sure if super() would work though and would be in the proper context. The copy pasta way would be the safest probably.

[chrisarcand]

I like the first solution.

[NickLaMuro]

Is there a way to do this that doesn't require updating this method? By that, I mean can we keep this method the same, and just include a wrapper method for render that includes invoking this method. Something like:

module JqueryRjs::RenderingHelper
  def render_with_update(options = {}, locals = {}, &block)
    if options == :update
      update_page(&block)
    else
      render_without_update(options, locals, &block)
    end

  if RUBY_VERSION >= '2'
    def render_without_update(*args)
      render(*args)
    end
  end
end

Not sure if this would work, but I am going off what you did.

Something to keep in mind is you are also changing the functionality of the function call with this newly defined method. In RUBY_VERSION < '2', it goes from the deleted code to this:

def render_with_update(options = {}, locals = {}, &block)
  if options == :update
    update_page(&block)
  end
  render_without_update(*args)
end

So render_without_update is always called instead of when options != :update. Is this expected?

[chrisarcand]

First code block: No, method must be named render to be properly inserted in the inheritance tree with prepend (must be able to call super)

Something to keep in mind is you are also changing the functionality of the function call...

No, not intended, good catch. That's a typo (a bad one).

[chrisarcand]

(Fixed)

[jrafanie]

@chrisarcand is there any chance to get this in an upstream? I'd imagine we're not the only ones getting deprecation warnings with rails 5.

[jrafanie]

The change looks good on the whole, I have no idea how to test that we didn't break something

[chrisarcand]

@jrafanie The 'official' fork we use is Akira's (https://github.com/amatsuda/jquery-rjs). We'll have to continue using either his or this fork, or cut our own renamed gem. I'd rather not do the later as we don't really 'support' a lot here and are moving from RJS. And the test suite is a mess, as I mentioned 😄

But I would throw a PR at Akira's fork just to see if it's desired there.

[chrisarcand]

The change looks good on the whole, I have no idea how to test that we didn't break something

Ditto, sort of. I tested with ManageIQ where @h-kataria pointed out that we use RJS. The RJS callers in the controller are hit as expected, but nothing seems to hit these particular patches. ¯(°_o)/¯

[chrisarcand]

I think it depends on if you buy into the B.S. argument I have been making this whole time 😉. That, and if it is worth the effort to do it this way.

Indeed 😄 So I wrote it all out and couldn't get over how terrible it looked. I see the point of not wanting the same conditional running with the same result and a conditional repeated in so many places to all achieve the same thing, so the most I did was just extract that to an interface and cache it (for whatever measly savings that may or may not give)

Considering we want to move away from using this (as everyone else should...), I'm calling it good at this if everyone's fine with it.

[djberg96]

Looks good! Please merge!

[chrisarcand]

I've determined that this patch is probably completely harmless to us as this code is very likely not even called and probably not even patched correctly anymore.

response_from_page was removed in the rails-dom-testing gem at some point. Rails 5 probably uses this. rails/rails-dom-testing@3b96877

Let's merge this to get rid of the deprecations warnings but this is yet another sign that we should be moving off RJS as soon as possible (or, embrace it and actually maintain this shell of properly running gem).

I'll add a warning to the method for future help.

[chrisarcand]

...nevermind, that's patched on in this gem here

[jrafanie]

LGTM, I don't have merge rights here. cc @Fryguy

[chrisarcand]

This is totally broken because the gem is totally broken. It doesn't properly alias_method_chain anything it whatsoever. I think I might just cut a single file with the crap we DO use (a few things), put that in MIQ, and delete this fork forever. Hold off for a bit longer.

[jrafanie]

ok

[chrisarcand]

Ok. I've decided, finally. I promise.

Merge this. I know for a fact the gem isn't actually doing anything in this area (broken even with old alias_method_chain) but extracting the actual code we need and pitching the rest is more a pedantic task than an actual useful one. No one should be using this gem past Rails 3, period.

Let's just merge this crap and get off RJS instead, then delete this fork from our brains forever.