Only add exist paths to reloading target

[y-yagi]

Currently, factories, test/factories and spec/factories are specified by default as reload watching paths.
In many applications, there are directories which do not exist (perhaps do not have spec if using minitest and do not have spec if using minitest).

In Rails 5 series, if specify a path that does not exist inEventedFileUpdateChecker, its parent is added to the watching path.
As a result, node_modules is also included in the watching path, and unexpectedly many directories and files are included in listen's watching targets. Also, if symlink is included in node_modules, it also causes warning of listen.

This issue is solved in Rails 6. However, since many applications use this gem in Rails 5 and below, it is good not to add directories that do not exist on the gem side.

Ref: rails/rails#32700

[houndci-bot]

Metrics/LineLength: Line is too long. [84/80]

[y-yagi]

The test is an error due to an incompatibility issue with Rails and the new version of sqlite3 gem.
Ref: rails/rails#35161

[composerinteralia]

Yeah, good catch. Now I remember seeing rails/rails#33822.

Is there a scenario where this fix could cause problems? If spring is running, then after that I add my first factory to spec/factories, will I need to run spring stop to get that factory to load?

And if that is the case, would it make sense to only do this filtering out if we are running a rails version before 6.0?

[composerinteralia]

I don't think this will quite work because the FactoryBot.definition_file_paths gets used for two things, both a directory and a file. So if spec/factories is in FactoryBot.definition_file_paths we watch both the spec/factories directory and also the spec/factories.rb file. With this change we would also stop watching spec/factories.rb if spec/factories doesn't exist. We may need to move this logic into the reloader class.

[y-yagi]

When checking the directory to watching, the same check is done for the parent directory of the file. Therefore, the same fix is necessary for the files.
https://github.com/rails/rails/blob/188ccf2f41b3d6bb77ba48b81343f80cf5615b75/activesupport/lib/active_support/evented_file_update_checker.rb#L126

checker = ActiveSupport::EventedFileUpdateChecker.new(%w(spec/factories.rb test/factories.rb), []) {}
checker.send(:directories_to_watch).include?(Rails.root)
#=> true 

[y-yagi]

Thanks for your review!

Yes, your concern is correct.
If spring is running with spec/factories not exist, spring must be restarted after spec/factories is created.
However, I think that this probably does not occur much. For example, if create a factory using scaffold, this will not happen because spring is restarted automatically(If you change the file under config, it will be restarted automatically).

But of course, it is possible to respond by checking the version of Rails. Should I do?

[composerinteralia]

I will trust your judgement on the scenario with spring. If an issue comes up, we can always address it at that time.

I think we will still have some failing tests once this gets rebased on master, possibly related to my point about FactoryBot.definition_file_paths (I saw failures locally relating to definition loading). Once we resolve those failures I think this will be good to merge and I will release 5.0.1.

[y-yagi]

Oh, sorry. I misunderstood definition_file_paths behavior. I fixed file / directory existing check and test pass now.

[composerinteralia]

Thanks! I'll have time to push the 5.0.1 release on Friday.

[y-yagi]

Thanks!