Remove support for liquid_methods Module extension

[fw42]

I think this is ugly and should be discouraged.

@arthurnn @pushrax @dylanahsmith

[pushrax]

I'm cool with this. Need to have a clear blurb stating the migration path - dunno if this goes in the changelog or elsewhere.

[dylanahsmith]

👍 this could easily live outside of liquid if anyone cares about it.

[tdegrunt]

This has just gone 'live' in version 4, though this makes sense, what's the suggested migration path? Or, how would one properly implement this without the use of liquid_methods?

[pushrax]

It's recommended to create drop classes explicitly. What it was doing under the hood was rather simple: https://github.com/Shopify/liquid/pull/568/files#diff-c54ab82e2cb4832484af67335d101514L45

[tdegrunt]

Allright, understood.

In the meantime if anyone wants a migration path (in case of Rails) drop this in a concern and include it in your models:

module LiquidMethods
  extend ActiveSupport::Concern

  module ClassMethods

    def liquid_methods(*allowed_methods)
      drop_class = eval "class #{self}::LiquidDropClass < Liquid::Drop; self; end"

      define_method :to_liquid do
        drop_class.new(self)
      end

      drop_class.class_eval do
        def initialize(object)
          @object = object
        end

        allowed_methods.each do |sym|
          define_method sym do
            @object.send sym
          end
        end
      end
    end
  end
end

[jaredbeck]

It's recommended to create drop classes explicitly.

Is this correct?

class Banana < ApplicationRecord
-  liquid_methods :variety, :mass, :deliciousness
+  class LiquidDropBanana < Liquid::Drop
+    def initialize(banana)
+      @banana = banana
+    end
+
+    delegate :variety, :mass, :deliciousness, to: :@banana
+  end
+
+  def to_liquid
+    LiquidDropBanana.new(self)
+  end

If so,

1. should LiquidDropBanana be in a separate file? Where? app/drops?
2. I couldn't find the word "drop" in the documentation. What is a drop?

Thanks.

[dylanahsmith]

should LiquidDropBanana be in a separate file? Where? app/drops?

Do whatever you feel is appropriate.

I couldn't find the word "drop" in the documentation. What is a drop?

https://github.com/Shopify/liquid/wiki/Introduction-to-Drops

[jaredbeck]

It's recommended to create drop classes explicitly.

Is this correct?

I did not receive an answer on this, but what I showed above passes my tests. ¯_(ツ)_/¯

should LiquidDropBanana be in a separate file? Where? app/drops?

Do whatever you feel is appropriate.

I went with app/drops. Seems to be OK.

I couldn't find the word "drop" in the documentation. What is a drop?

https://github.com/Shopify/liquid/wiki/Introduction-to-Drops

Thanks, I was looking in http://docs.shopify.com/themes/liquid-basics

Other than this issue, upgrade to liquid 4 seems to have gone well, will report back if I find anything else.

[brendon]

Say I had a relation off a parent drop (Rails). The parent drop has a children method. Would it be better to define to_liquid on the child model which in turn creates a new child drop with the child and have the children method just return an array of children, or should the children method map the children, wrapping them all in child drops? The second option removed the need for the model to know about liquid, and I'm currently favouring that but wanted to see what the general consensus was.

[pushrax]

It would be more robust to define to_liquid on the child model.