Improve after_reload issues (#1692)

* Improve `after_reload` issues
* Make `after_reload` run only if paths were updated.
* Pass the actual reloaded paths to the `after_reload` blocks.

* Fix `after_reload` spec

* Remove hardcoded filename, and check reload isn't called

* Undo oops version change

* Add rspec for existing after_reload blocks without input param

* Guard against passing lambdas

* Reset after_reload block when containing file reloaded

Co-authored-by: Jordan Owens <jkowens@gmail.com>

sinatra-contrib/lib/sinatra/reloader.rb

@@ -215,7 +215,7 @@ module Sinatra
     # Allow a block to be executed after any file being reloaded
     @@after_reload = []
     def after_reload(&block)
-      @@after_reload  << block
+      @@after_reload << block
     end
 
     # When the extension is registered it extends the Sinatra application
@@ -242,14 +242,26 @@ module Sinatra
     # Reloads the modified files, adding, updating and removing the
     # needed elements.
     def self.perform(klass)
+      reloaded_paths = []
       Watcher::List.for(klass).updated.each do |watcher|
         klass.set(:inline_templates, watcher.path) if watcher.inline_templates?
         watcher.elements.each { |element| klass.deactivate(element) }
+        # Deletes all old elements.
+        watcher.elements.delete_if { true }
         $LOADED_FEATURES.delete(watcher.path)
         require watcher.path
         watcher.update
+        reloaded_paths << watcher.path
+      end
+      return if reloaded_paths.empty?
+      @@after_reload.each do |block|
+        block.arity != 0  ? block.call(reloaded_paths) : block.call
+      end
+      # Prevents after_reload from increasing each time it's reloaded.
+      @@after_reload.delete_if do |blk|
+        path, _ = blk.source_location
+        path && reloaded_paths.include?(path)
       end
-      @@after_reload.each(&:call)
     end
 
     # Contains the methods defined in Sinatra::Base that are overridden.

sinatra-contrib/spec/reloader_spec.rb

@@ -418,6 +418,7 @@ RSpec.describe Sinatra::Reloader do
 
   describe ".after_reload" do
     before(:each) do
+      $reloaded = nil
       setup_example_app(:routes => ['get("/foo") { Foo.foo }'])
       @foo_path = File.join(tmp_dir, 'foo.rb')
       update_file(@foo_path) do |f|
@@ -429,15 +430,46 @@ RSpec.describe Sinatra::Reloader do
     end
 
     it "allows block execution after reloading files" do
-      app_const.after_reload do
-        $reloaded = true
+      app_const.after_reload do |files|
+        $reloaded = files
       end
       expect($reloaded).to eq(nil)
       expect(get('/foo').body.strip).to eq('foo')
+      expect($reloaded).to eq(nil) # after_reload was not called
       update_file(@foo_path) do |f|
         f.write 'class Foo; def self.foo() "bar" end end'
       end
-      expect($reloaded).to eq(true)
+      expect(get("/foo").body.strip).to eq("bar") # Makes the reload happen
+      expect($reloaded.size).to eq(1)
+      expect(File.basename($reloaded[0])).to eq(File.basename(@foo_path))
+    end
+
+    it "does not break block without input param" do
+      app_const.after_reload do
+        $reloaded = "worked without param"
+      end
+      expect($reloaded).to eq(nil)
+      expect(get('/foo').body.strip).to eq('foo')
+      expect($reloaded).to eq(nil) # after_reload was not called
+      update_file(@foo_path) do |f|
+        f.write 'class Foo; def self.foo() "bar" end end'
+      end
+      expect { get("/foo") }.to_not raise_error # Makes the reload happen
+      expect($reloaded).to eq("worked without param")
+    end
+
+    it "handles lambdas with arity 0" do
+      user_proc = -> { $reloaded = "lambda?=true arity=0" }
+      expect { user_proc.call(1) }.to raise_error(ArgumentError) # What we avoid
+      app_const.after_reload(&user_proc)
+      expect($reloaded).to eq(nil)
+      expect(get('/foo').body.strip).to eq('foo')
+      expect($reloaded).to eq(nil) # after_reload was not called
+      update_file(@foo_path) do |f|
+        f.write 'class Foo; def self.foo() "bar" end end'
+      end
+      expect { get("/foo") }.to_not raise_error # Makes the reload happen
+      expect($reloaded).to eq("lambda?=true arity=0")
     end
   end