Fix AssetsPrecompile#symlink_file logic

When we changed this method in #513 we introduced a regression due
to the difference between calling the shell's `ln -s` command and
using Ruby's `File.symlink` command.

Specifically, the former would not error if the symlink already
existed, while the latter did throw an error. Because it is
sometimes the case that the symlink will already exist, throwing
in this case is not desirable.

This should have not been a problem, however, as this scenario
was supposed to have been properly handled, but that code was not
correct. This commit fixes that code.

Author: robwise

lib/react_on_rails/assets_precompile.rb

@@ -1,5 +1,7 @@
 module ReactOnRails
   class AssetsPrecompile
+    class SymlinkTargetDoesNotExistException < StandardError; end
+
     # Used by the rake task
     def default_asset_path
       dir = File.join(Rails.configuration.paths["public"].first,
@@ -20,30 +22,35 @@ def initialize(assets_path: nil,
     def symlink_file(target, symlink)
       target_path = @assets_path.join(target)
       symlink_path = @assets_path.join(symlink)
+
       target_exists = File.exist?(target_path)
+      raise SymlinkTargetDoesNotExistException, "Target Path was: #{target_path}" unless target_exists
 
       # File.exist?(symlink_path) will check the file the sym is pointing to is existing
       # File.lstat(symlink_path).symlink? confirms that this is a symlink
-      symlink_already_there_and_valid = File.exist?(symlink_path) &&
-                                        File.lstat(symlink_path).symlink?
-      if symlink_already_there_and_valid
-        puts "React On Rails: Digested #{symlink} already exists indicating #{target} did not change."
-      elsif target_exists
-        if File.exist?(symlink_path) && File.lstat(symlink_path).symlink?
-          puts "React On Rails: Removing invalid symlink #{symlink_path}"
-          `cd #{@assets_path} && rm #{symlink}`
-        end
-        # Might be like:
-        # "images/5cf5db49df178f9357603f945752a1ef.png":
-        # "images/5cf5db49df178f9357603f945752a1ef-033650e1d6193b70d59bb60e773f47b6d9aefdd56abc7cc.png"
-        # need to cd to directory and then symlink
-        target_sub_path, _divider, target_filename = target.rpartition("/")
-        _symlink_sub_path, _divider, symlink_filename = symlink.rpartition("/")
-        puts "React On Rails: Symlinking \"#{target}\" to \"#{symlink}\""
-        dest_path = File.join(@assets_path, target_sub_path)
-        FileUtils.chdir(dest_path) do
-          File.symlink(target_filename, symlink_filename)
-        end
+      valid_symlink_already_exists = File.exist?(symlink_path) && File.lstat(symlink_path).symlink?
+
+      if valid_symlink_already_exists
+        puts "React On Rails: Digested version of #{symlink} already exists indicating #{target} did not change."
+        return
+      end
+
+      if file_or_symlink_exists_at_path(symlink_path)
+        puts "React On Rails: Removing existing invalid symlink or file #{symlink_path}"
+        `rm -f "#{symlink_path}"`
+      end
+
+      # Might be like:
+      # "images/5cf5db49df178f9357603f945752a1ef.png":
+      # "images/5cf5db49df178f9357603f945752a1ef-033650e1d6193b70d59bb60e773f47b6d9aefdd56abc7cc.png"
+      # need to cd to directory and then symlink
+      target_sub_path, _divider, target_filename = target.rpartition("/")
+      _symlink_sub_path, _divider, symlink_filename = symlink.rpartition("/")
+      dest_path = File.join(@assets_path, target_sub_path)
+
+      puts "React On Rails: Symlinking \"#{target}\" to \"#{symlink}\""
+      FileUtils.chdir(dest_path) do
+        File.symlink(target_filename, symlink_filename)
       end
     end
 
@@ -74,8 +81,10 @@ def symlink_non_digested_assets
           # already been symlinked by Webpack
           symlink_file(rails_digested_filename, original_filename)
 
-          # We want the gz ones as well
-          symlink_file("#{rails_digested_filename}.gz", "#{original_filename}.gz")
+          # We want the gz ones as well if they exist
+          if File.exist?(@assets_path.join("#{rails_digested_filename}.gz"))
+            symlink_file("#{rails_digested_filename}.gz", "#{original_filename}.gz")
+          end
         end
       end
     end
@@ -108,5 +117,14 @@ def clobber
         puts "Could not find generated_assets_dir #{dir} defined in react_on_rails initializer: "
       end
     end
+
+    private
+
+    def file_or_symlink_exists_at_path(path)
+      File.lstat(path)
+      true
+    rescue
+      false
+    end
   end
 end

spec/react_on_rails/assets_precompile_spec.rb

@@ -54,6 +54,14 @@ module ReactOnRails
         expect(File.identical?(assets_path.join(filename),
                                assets_path.join(digest_filename))).to be true
       end
+
+      context "when no file exists at the target path" do
+        it "raises a ReactOnRails::AssetsPrecompile::SymlinkTargetDoesNotExistException" do
+          expect do
+            AssetsPrecompile.new(assets_path: assets_path).symlink_file("non_existent", "non_existent-digest")
+          end.to raise_exception(AssetsPrecompile::SymlinkTargetDoesNotExistException)
+        end
+      end
     end
 
     describe "symlink_non_digested_assets" do
@@ -69,35 +77,23 @@ module ReactOnRails
                              symlink_non_digested_assets_regex: Regexp.new('.*\.js$'))
       end
 
-      context "correct nondigest filename" do
-        it "creates valid symlink" do
-          FileUtils.touch assets_path.join(digest_filename)
-          checker.symlink_non_digested_assets
-
-          expect(assets_path.join(nondigest_filename).lstat.symlink?).to be true
-          expect(File.identical?(assets_path.join(nondigest_filename),
-                                 assets_path.join(digest_filename))).to be true
-        end
-      end
-
-      context "zipped nondigest filename" do
-        it "creates valid symlink" do
-          FileUtils.touch assets_path.join("#{digest_filename}.gz")
-          checker.symlink_non_digested_assets
+      it "creates a symlink with the original filename that points to the digested filename" do
+        FileUtils.touch assets_path.join(digest_filename)
+        checker.symlink_non_digested_assets
 
-          expect(assets_path.join("#{nondigest_filename}.gz").lstat.symlink?).to be true
-          expect(File.identical?(assets_path.join("#{nondigest_filename}.gz"),
-                                 assets_path.join("#{digest_filename}.gz"))).to be true
-        end
+        expect(assets_path.join(nondigest_filename).lstat.symlink?).to be true
+        expect(File.identical?(assets_path.join(nondigest_filename),
+                               assets_path.join(digest_filename))).to be true
       end
 
-      context "wrong nondigest filename" do
-        it "should not create symlink" do
-          FileUtils.touch assets_path.join("alfa.12345.jsx")
-          checker.symlink_non_digested_assets
+      it "creates a symlink with the original filename plus .gz that points to the gzipped digested filename" do
+        FileUtils.touch assets_path.join(digest_filename)
+        FileUtils.touch assets_path.join("#{digest_filename}.gz")
+        checker.symlink_non_digested_assets
 
-          expect(assets_path.join("alfa.jsx")).not_to exist
-        end
+        expect(assets_path.join("#{nondigest_filename}.gz").lstat.symlink?).to be true
+        expect(File.identical?(assets_path.join("#{nondigest_filename}.gz"),
+                               assets_path.join("#{digest_filename}.gz"))).to be true
       end
     end