FIX: Recompile extra_js theme assets when COMPILER_VERSION changes (#7897)

FIX: Recompile extra_js theme assets when COMPILER_VERSION changes (#7897)

diff --git a/app/models/theme.rb b/app/models/theme.rb
index 798bb2e..72e9471 100644
--- a/app/models/theme.rb
+++ b/app/models/theme.rb
@@ -63,6 +63,15 @@ class Theme < ActiveRecord::Base
     settings_field&.ensure_baked! # Other fields require setting to be **baked**
     theme_fields.each(&:ensure_baked!)
 
+    update_javascript_cache!
+
+    remove_from_cache!
+    clear_cached_settings!
+    ColorScheme.hex_cache.clear
+    notify_theme_change(with_scheme: notify_with_scheme)
+  end
+
+  def update_javascript_cache!
     all_extra_js = theme_fields.where(target_id: Theme.targets[:extra_js]).pluck(:value_baked).join("\n")
     if all_extra_js.present?
       js_compiler = ThemeJavascriptCompiler.new(id, name)
@@ -73,11 +82,6 @@ class Theme < ActiveRecord::Base
     else
       javascript_cache&.destroy!
     end
-
-    remove_from_cache!
-    clear_cached_settings!
-    ColorScheme.hex_cache.clear
-    notify_theme_change(with_scheme: notify_with_scheme)
   end
 
   after_destroy do
@@ -288,6 +292,12 @@ class Theme < ActiveRecord::Base
 
   def self.resolve_baked_field(theme_ids, target, name)
     if target == :extra_js
+      require_rebake = ThemeField.where(theme_id: theme_ids, target_id: Theme.targets[:extra_js]).
+        where("compiler_version <> ?", ThemeField::COMPILER_VERSION)
+      require_rebake.each { |tf| tf.ensure_baked! }
+      require_rebake.map(&:theme_id).uniq.each do |theme_id|
+        Theme.find(theme_id).update_javascript_cache!
+      end
       caches = JavascriptCache.where(theme_id: theme_ids)
       caches = caches.sort_by { |cache| theme_ids.index(cache.theme_id) }
       return caches.map { |c| "<script src='#{c.url}'></script>" }.join("\n")
diff --git a/spec/models/theme_spec.rb b/spec/models/theme_spec.rb
index 0431413..d760a5f 100644
--- a/spec/models/theme_spec.rb
+++ b/spec/models/theme_spec.rb
@@ -674,4 +674,39 @@ HTML
       expect(Theme.list_baked_fields([theme.id, theme2.id], :translations, 'fr').map(&:id)).to contain_exactly(fr_translation.id, en_translation2.id)
     end
   end
+
+  describe "automatic recompile" do
+    it 'must recompile after bumping theme_field version' do
+      def stub_const(target, const, value)
+        old = target.const_get(const)
+        target.send(:remove_const, const)
+        target.const_set(const, value)
+        yield
+      ensure
+        target.send(:remove_const, const)
+        target.const_set(const, old)
+      end
+
+      child.set_field(target: :common, name: "header", value: "World")
+      child.set_field(target: :extra_js, name: "test.js.es6", value: "const hello = 'world';")
+      child.save!
+
+      first_common_value = Theme.lookup_field(child.id, :desktop, "header")
+      first_extra_js_value = Theme.lookup_field(child.id, :extra_js, nil)
+
+      stub_const(ThemeField, :COMPILER_VERSION, "SOME_NEW_HASH") do
+        second_common_value = Theme.lookup_field(child.id, :desktop, "header")
+        second_extra_js_value = Theme.lookup_field(child.id, :extra_js, nil)
+
+        new_common_compiler_version = ThemeField.find_by(theme_id: child.id, name: "header").compiler_version
+        new_extra_js_compiler_version = ThemeField.find_by(theme_id: child.id, name: "test.js.es6").compiler_version
+
+        expect(first_common_value).to eq(second_common_value)
+        expect(first_extra_js_value).to eq(second_extra_js_value)
+
+        expect(new_common_compiler_version).to eq("SOME_NEW_HASH")
+        expect(new_extra_js_compiler_version).to eq("SOME_NEW_HASH")
+      end
+    end
+  end
 end

GitHub sha: ed5b31f4

Would be safer to use find_each

Theme.where(id: require_rebake.map(&:theme_id).uniq).find_each(&:update_javascript_cache!)

I don’t mind changing it. But why would it be safer? From the find_each documentation

If you just need to loop over less than 1000 records, it’s probably better just to use the regular find methods.

If someone has 1000 theme components attached to a single theme, I think they will have bigger problems :wink:

1 Like