Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Load order of overrides on different systems #162

Open
gaqzi opened this issue Jun 27, 2016 · 0 comments
Open

Load order of overrides on different systems #162

gaqzi opened this issue Jun 27, 2016 · 0 comments

Comments

@gaqzi
Copy link

gaqzi commented Jun 27, 2016

I recently noticed a weird behavior of Deface when one of our overrides was doing text: '<%= render partial: "...." %>' instead of partial: '.....'.

We only noticed this issue when it got into production on Linux and figured it would be something with sort order and started looking at how Deface loads the modules.

On my Mac I'm always seeing the overrides being returned in the same order, alphabetical. It also performs the overrides in alphabetical order when I have partial as the action, but if I change it to text it'll be in a semi-random order.

To help debug this I did some monkey patching which shows the order (and some testing with setting the order seems to make it pass reliably on Heroku):

Deface::Override.class_eval do
  def self.find(details)
#       .sort { |a, b| [a.sequence, a.name] <=> [b.sequence, b.name] }
    super(details)
      .tap do |value|
        Rails.logger.error("Order of deface being put in: '#{value.map &:name}'") unless value.blank?
      end
  end
end

The default, expected, order: Order of deface being put in: '["add_terms_checkbox", "make_checkout_update_buttons_big_on_mobile", "make_continue_shopping_button_big_on_mobile", "remove_empty_cart_button", "replace_cart_table_with_responsive_containers"]'
The out of order: Order of deface being put in: '["make_checkout_update_buttons_big_on_mobile", "make_continue_shopping_button_big_on_mobile", "remove_empty_cart_button", "replace_cart_table_with_responsive_containers", "add_terms_checkbox"]'

For these tests, none of these overrides have a sequence. To work around this and to ensure it continues to work I'm adding in sequence to these templates.

All of this is a bit iffy as I've not been able to understand what's going on, and even worse, all the tests were passing on our CI which is also running on Linux (but whatever the difference between CircleCi and Heroku is…). And whether the order from find is changing stuff I can't make out. But after having put in .sort { |a, b| [a.sequence, a.name] <=> [b.sequence, b.name] } to my patched version of find it seems to have been doing better.

My suggestion would be to add in the sort above as it would make the load order consistent between different operating systems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant