Issue with conditionals when extending base Serializable::Resource class directly in 0.2.1

[richmolj]

I believe this commit, which changed from prepend to extend, broke the conditional support. I noticed this when trying to upgrade jsonapi_compliable - my if procs no longer get called, which broke extra_fields support.

The expectation is that I will get this overridden #requested_attributes, but debugging the code I see this is never called. The original #requested_attributes method is called.

I'm not sure how specs are passing, because this script mimics the same pattern and illustrates the problem:

class Foo
  def bar
    'original bar'
  end
end

module Mix
  def self.extended(klass)
    klass.class_eval do
      include InstanceMethods
    end
  end

  module InstanceMethods
    def bar
      'overridden bar'
    end
  end
end
Foo.extend Mix
puts Foo.new.bar #=> 'original'

[beauby]

Yeah you shouldn't extend the Serializable::Resource class itself directly but a subclass.

[richmolj]

How is that supposed to work for something like jsonapi_suite? I have existing applications with ~100 serializers inheriting from Serializable::Resource, do I now have to add another serializer class to jsonapi_suite and switch everything to inherit from that?

[beauby]

Hmm I wasn't aware that you were customizing the base class. Would you mind issuing a PR re-modifying extensions to use prepend instead?

[richmolj]

Would that be more or less reverting e95fd7e?

[richmolj]

@beauby not wild about it, but I solved this by extending after inheriting:

JSONAPI::Serializable::Resource.class_eval do
  def self.inherited(klass)
    super
    klass.class_eval do
      extend JSONAPI::Serializable::Resource::ConditionalFields
    end
  end
end

here. Not wild about it but works. Up to you if you want to keep this pattern or move back to prepend

[beauby]

Smart workaround! I'm currently away from machines but I'm keeping this under my radar

On Wed, 9 Aug 2017 at 16:21, Lee Richmond ***@***.***> wrote: @beauby <https://github.com/beauby> not wild about it, but I solved this by extending after inheriting: JSONAPI::Serializable::Resource.class_eval do def self.inherited(klass) super klass.class_eval do extend JSONAPI::Serializable::Resource::ConditionalFields end endend here <https://github.com/richmolj/jsonapi_compliable/blob/deed7ed0dba6ef2dcb54c23158992ec9ddf05c11/lib/jsonapi_compliable/extensions/extra_attribute.rb#L59-L68>. Not wild about it but works. Up to you if you want to keep this pattern or move back to prepend — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#80 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACHPYtFpHVFwEmJzfgeVabEpCE_fZ1Drks5sWcCCgaJpZM4OuvcD> .

-- Lucas Hosseini lucas.hosseini@gmail.com

[daniel-sullivan]

I've just put through a PR #81 if anyone's interested which keeps the extend in the serializer (base) class but moves back to using prepend for the inclusion of the InstanceMethods in the module_eval fixing the JSONAPI::Serializable::Resource precedence issue.

Tests seem to pass OK (I don't really have time at the moment to go through and check there's full coverage). Using this fix successfully under the example of (this is part of a jsonapi_suite based project):

class SerializableCompany < JSONAPI::Serializable::Resource
  extend JSONAPI::Serializable::Resource::ConditionalFields

  type :companies

  attributes :name, :phone

  attribute :address, unless: -> { @object.address.private? }

  link :self do
      @url_helpers.company_url(@object.id)
  end
end