Should use the most recent file when multiple sprockets manifest files are found

[ynonp]

A specific setup I had combining ReactOnRails with capistrano-rails had temporarily put multiple sprockets manifest files in public/assets when assets:precompile task was called.

This failed in file lib/react_on_rails/assets_precompile.rb line 75:

manifest_path = manifest_glob.first

A better approach would be to take the most recent one with:

manifest_path = manifest_glob.sort_by { |name| File.mtime(name) }.last

(Or raise an exception explaining the problem when multiple sprockets manifest files are found)

[justin808]

@ynonp should we have a PR for this?

@robwise, @samnang any opinion?

[robwise]

I can't really remember the details of why we use a glob instead of an exact file path in the first place, like what is the use case for having manifest files with varying names?

[ynonp]

I think part of the name of the manifest file is a hash code, and it's hard to know that hash. Anyways it's just one line, the code I wrote above works and takes the most recent manifest file, which is always a better guess than the first in the glob list

[ynonp]

I could make it a PR if you prefer. Just let me know

[justin808]

@ynonp Yes, please make a PR and make sure some test fails without and the same test passes with your change. And add an entry to the CHANGELOG.md.

[justin808]

@ynonp Any word on making a PR?

[justin808]

@samnang, @mapreal19 agree this seems to make sense?

manifest_path = manifest_glob.sort_by { |name| File.mtime(name) }.last

[mapreal19]

@justin808 yeah, we should just extract the method for hinting the intent by @ynonp. Something like take_most_recent_manifest_path

[justin808]

Fixed in #1064