Coverage increased (+0.02%) to 82.316% when pulling 6291e54 on dzirtusss:master into 63436ab on shakacode:master.

@dzirtusss Tip: Feel free to review your own PR first putting in questions and comments. These are useful to the reviewer and it's a place to put discussions/comments that don't deserve to be in code comments.

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

lib/react_on_rails/assets_precompile.rb, line 44 [r1] (raw file):

        puts "React On Rails: Symlinking \"#{target}\" to \"#{symlink}\""
        dest_path = File.join(@assets_path, target_sub_path)
        FileUtils.chdir(dest_path)

we should probably use the block syntax so that the chdir only applies to the code inside of the block.

lib/react_on_rails/assets_precompile.rb, line 45 [r1] (raw file):

        dest_path = File.join(@assets_path, target_sub_path)
        FileUtils.chdir(dest_path)
        File.symlink(File.expand_path(target_filename), File.expand_path(symlink_filename))

be careful with the file expand

we need a test to ensure that we're never using absolute paths

I suspect that you don't need the expand_path.

please be sure to verify this in the tests

spec/react_on_rails/assets_precompile_spec.rb, line 28 [r1] (raw file):

                                                               assets_path.join("test images")))
        digest_filename = "#{filename} digest"
        AssetsPrecompile.new(assets_path: assets_path)

if you ever check a symlink into github, then you can see the difference between the symlink that is the minimum relative path versus a full path.

spec/react_on_rails/assets_precompile_spec.rb, line 33 [r1] (raw file):

        expect(assets_path.join(digest_filename).lstat.symlink?).to be true
        expect(File.identical?(assets_path.join(filename),
                               assets_path.join(digest_filename))).to be true

Ideally, I'd like to see new tests with a file name that does not have spaces, so we can run the tests without the fix and see just these tests fail.

Comments from Reviewable

Coverage increased (+0.02%) to 82.316% when pulling d03cb71 on dzirtusss:master into c77d3b6 on shakacode:master.

Thanks @dzirtusss

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

lib/react_on_rails/assets_precompile.rb, line 45 [r1] (raw file):

spec/react_on_rails/assets_precompile_spec.rb, line 31 [r2] (raw file):

        expect(File.readlink(assets_path.join(digest_filename)).to_s)
          .not_to eq(File.expand_path(assets_path.join(filename)).to_s)
      end

CC: @alleycat-at-git Check this out.

Comments from Reviewable

@dzirtusss Please see https://github.com/shakacode/react_on_rails/blob/master/CONTRIBUTING.md.

We need to update the CHANGELOG.md.

Please add that entry and rebase on top of master.

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

lib/react_on_rails/assets_precompile.rb, line 44 [r1] (raw file):

lib/react_on_rails/assets_precompile.rb, line 45 [r1] (raw file):

spec/react_on_rails/assets_precompile_spec.rb, line 28 [r1] (raw file):

spec/react_on_rails/assets_precompile_spec.rb, line 33 [r1] (raw file):

Comments from Reviewable

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

lib/react_on_rails/assets_precompile.rb, line 45 [r1] (raw file):

Comments from Reviewable

Coverage increased (+0.02%) to 82.36% when pulling 357a36d on dzirtusss:master into a22abeb on shakacode:master.

Done

Review status: 2 of 3 files reviewed at latest revision, 5 unresolved discussions.

Comments from Reviewable

Coverage increased (+0.02%) to 82.36% when pulling 1a4eb4a on dzirtusss:master into a22abeb on shakacode:master.

Coverage increased (+0.02%) to 82.36% when pulling 1a4eb4a on dzirtusss:master into a22abeb on shakacode:master.

Merge commits

Review status: 2 of 3 files reviewed at latest revision, 5 unresolved discussions.

Comments from Reviewable

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

spec/react_on_rails/assets_precompile_spec.rb, line 23 [r3] (raw file):

      end

      it "doesn't create global symlink" do

"creates a relative symlink, rather than an absolute path symlink"

spec/react_on_rails/assets_precompile_spec.rb, line 30 [r3] (raw file):

        expect(File.readlink(assets_path.join(digest_filename)).to_s)
          .not_to eq(File.expand_path(assets_path.join(filename)).to_s)

Expect the readlink to equal the digest_filename?

If we test for equality, that's a bit more of a sure thing.

Only having an "expectation not" can result in a test that passes due to an error in writing the test.

Comments from Reviewable

Review status: all files reviewed at latest revision, 3 unresolved discussions.

spec/react_on_rails/assets_precompile_spec.rb, line 30 [r3] (raw file):

Expect the readlink to equal the digest_filename?

Expect the readlink to equal the filename?

Comments from Reviewable

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

spec/react_on_rails/assets_precompile_spec.rb, line 23 [r3] (raw file):

spec/react_on_rails/assets_precompile_spec.rb, line 30 [r3] (raw file):

Comments from Reviewable

@dzirtusss Please try to reopen this one.