Reuse "main" reloader for ActionView expiration

Co-authored-by: Matthew Draper <matthew@trebex.net>

actionview/lib/action_view/cache_expiry.rb

@@ -1,65 +1,65 @@
 # frozen_string_literal: true
 
 module ActionView
-  class CacheExpiry
-    class Executor
-      def initialize(watcher:)
-        @execution_lock = Concurrent::ReentrantReadWriteLock.new
-        @cache_expiry = ViewModificationWatcher.new(watcher: watcher) do
-          clear_cache
+  module CacheExpiry # :nodoc: all
+    class ViewReloader
+      def initialize(watcher:, &block)
+        @mutex = Mutex.new
+        @watcher_class = watcher
+        @watched_dirs = nil
+        @watcher = nil
+        @previous_change = false
+
+        rebuild_watcher
+
+        _self = self
+        ActionView::ViewPaths::Registry.singleton_class.set_callback(:build_file_system_resolver, :after) do
+          _self.send(:rebuild_watcher)
         end
       end
 
-      def run
-        ActiveSupport::Dependencies.interlock.permit_concurrent_loads do
-          @cache_expiry.execute_if_updated
-          @execution_lock.acquire_read_lock
-        end
+      def updated?
+        @previous_change || @watcher.updated?
       end
 
-      def complete(_)
-        @execution_lock.release_read_lock
+      def execute
+        watcher = nil
+        @mutex.synchronize do
+          @previous_change = false
+          watcher = @watcher
+        end
+        watcher.execute
       end
 
       private
-        def clear_cache
-          @execution_lock.with_write_lock do
-            ActionView::LookupContext::DetailsKey.clear
-          end
+        def reload!
+          ActionView::LookupContext::DetailsKey.clear
         end
-    end
 
-    class ViewModificationWatcher
-      def initialize(watcher:, &block)
-        @watched_dirs = nil
-        @watcher_class = watcher
-        @watcher = nil
-        @mutex = Mutex.new
-        @block = block
-      end
+        def rebuild_watcher
+          @mutex.synchronize do
+            old_watcher = @watcher
 
-      def execute_if_updated
-        @mutex.synchronize do
-          watched_dirs = dirs_to_watch
-          return if watched_dirs.empty?
+            if @watched_dirs != dirs_to_watch
+              @watched_dirs = dirs_to_watch
+              new_watcher = @watcher_class.new([], @watched_dirs) do
+                reload!
+              end
+              @watcher = new_watcher
 
-          if watched_dirs != @watched_dirs
-            @watched_dirs = watched_dirs
-            @watcher = @watcher_class.new([], watched_dirs, &@block)
-            @watcher.execute
-          else
-            @watcher.execute_if_updated
+              # We must check the old watcher after initializing the new one to
+              # ensure we don't miss any events
+              @previous_change ||= old_watcher&.updated?
+            end
           end
         end
-      end
 
-      private
         def dirs_to_watch
-          all_view_paths.grep(FileSystemResolver).map!(&:path).tap(&:uniq!).sort!
+          all_view_paths.uniq.sort
         end
 
         def all_view_paths
-          ActionView::ViewPaths.all_view_paths.flat_map(&:paths)
+          ActionView::ViewPaths::Registry.all_file_system_resolvers.map(&:path)
         end
     end
   end

actionview/lib/action_view/path_set.rb

@@ -67,7 +67,7 @@ def typecast(paths)
         paths.map do |path|
           case path
           when Pathname, String
-            FileSystemResolver.new path.to_s
+            ViewPaths::Registry.file_system_resolver(path.to_s)
           when Resolver
             path
           else

actionview/lib/action_view/railtie.rb

@@ -107,7 +107,14 @@ class Railtie < Rails::Engine # :nodoc:
       end
 
       unless enable_caching
-        app.executor.register_hook ActionView::CacheExpiry::Executor.new(watcher: app.config.file_watcher)
+        view_reloader = ActionView::CacheExpiry::ViewReloader.new(watcher: app.config.file_watcher)
+
+        app.reloaders << view_reloader
+        view_reloader.execute
+        app.reloader.to_run do
+          require_unload_lock!
+          view_reloader.execute
+        end
       end
     end
 

actionview/lib/action_view/view_paths.rb

@@ -87,16 +87,14 @@ def self.set_view_paths(klass, paths)
         @view_paths_by_class[klass] = paths
       end
 
-      def self.all_resolvers
-        @all_view_paths.values.map(&:to_a).flatten.uniq
-      end
-
       def self.file_system_resolver(path)
         path = File.expand_path(path)
         resolver = @file_system_resolvers[path]
         unless resolver
-          resolver = @file_system_resolvers.fetch_or_store(path) do
-            FileSystemResolver.new(path)
+          run_callbacks(:build_file_system_resolver) do
+            resolver = @file_system_resolvers.fetch_or_store(path) do
+              FileSystemResolver.new(path)
+            end
           end
         end
         resolver

railties/test/application/per_request_digest_cache_test.rb

@@ -62,7 +62,7 @@ def index
   end
 
   test "template digests are cleared before a request" do
-    assert_called(ActionView::LookupContext::DetailsKey, :clear, times: 3) do
+    assert_called(ActionView::LookupContext::DetailsKey, :clear, times: 2) do
       get "/customers"
       assert_equal 200, last_response.status
     end

railties/test/application/view_reloading_test.rb

@@ -0,0 +1,56 @@
+# frozen_string_literal: true
+
+require "isolation/abstract_unit"
+require "rack/test"
+
+module ApplicationTests
+  class ViewReloadingTest < ActiveSupport::TestCase
+    include ActiveSupport::Testing::Isolation
+    include Rack::Test::Methods
+
+    def setup
+      build_app
+
+      app_file "config/routes.rb", <<-RUBY
+        Rails.application.routes.draw do
+          get 'pages/:id', to: 'pages#show'
+        end
+      RUBY
+
+      app_file "app/controllers/pages_controller.rb", <<-RUBY
+        class PagesController < ApplicationController
+          layout false
+
+          def show
+          end
+        end
+      RUBY
+    end
+
+    def teardown
+      teardown_app
+    end
+
+    test "views are reloaded" do
+      app_file "app/views/pages/show.html.erb", <<-RUBY
+        Before!
+      RUBY
+
+      ENV["RAILS_ENV"] = "development"
+      require "#{app_path}/config/environment"
+
+      get "/pages/foo"
+      get "/pages/foo"
+      assert_equal 200, last_response.status, last_response.body
+      assert_equal "Before!", last_response.body.strip
+
+      app_file "app/views/pages/show.html.erb", <<-RUBY
+        After!
+      RUBY
+
+      get "/pages/foo"
+      assert_equal 200, last_response.status
+      assert_equal "After!", last_response.body.strip
+    end
+  end
+end