hyperstack icon indicating copy to clipboard operation
hyperstack copied to clipboard

loading? should work properly with ActiveRecord instances

Open catmando opened this issue 5 years ago • 0 comments

If foo is some instance of an activerecord model then foo.loading? should return true if any of the attributes in foo are loading.

Right now loading? only works with attributes, but not the entire record, no on collections of records

module ActiveRecord
  module InstanceMethods

    def loading?
      to_check = [self]
      next_check = 0
      while next_check < to_check.length
        to_check[next_check].attributes.values.each do |value|
          if value.is_a? ActiveRecord::Base
            to_check << value unless to_check.include? value
          elsif value.loading?
            puts "loading is true"
            return true
          end
        end
        next_check += 1
      end
      false
    end

    def loaded?
      !loading?
    end
end

Likewise for collections... but to make this more efficient you probably want the base method to be something like array_loading?(array) -> returns true/false, using the above algorithm, but initializing the array from the param.

That way the collection can call array_loading and not worry about re checking records inside the tree more than once.

BUT the other big catch is that the set_attribute_change_status_and_notify internal method needs to change so that it only checks no active record models.

module ReactiveRecord
  class Base # was module Setters
    def attribute_loaded?(current_value)
      current_value.is_a?(ActiveRecord::Base) || current_value.loaded?
    end

    def set_attribute_change_status_and_notify(attr, changed, new_value)
      if @virgin
        @attributes[attr] = new_value
      else
        change_status_and_notify_helper(attr, changed) do |had_key, current_value|
          @attributes[attr] = new_value
          if !data_loading? ||  # following line was current_value.loaded? 
             (on_opal_client? && had_key && attribute_loaded?(current_value) && current_value != new_value)
            Hyperstack::Internal::State::Variable.set(self, attr, new_value, data_loading?)
          end
        end
      end
    end
  end
end

This change is the right thing to do, but is going to be a bit problematic for existing HS systems, as people have probably come to depend on some_model.loading? working the way it does today.

catmando avatar Jul 01 '20 20:07 catmando