mikker / passwordless

🗝 Authentication for your Rails app without the icky-ness of passwords
MIT License
1.26k stars 87 forks source link

use ActiveSupport::FileUpdateChecker in the development environment. #111

Closed madogiwa0124 closed 2 years ago

madogiwa0124 commented 2 years ago

This is a PR to changed to ActiveSupport::FileUpdateChecker from ActiveSupport::EventedFileUpdateChecker.

How about this PR?

Description

The ActiveSupport::EventedFileUpdateChecker depends on listen, but since it is not installed, an exception is thrown when trying to start the development environment.

bin/rails s
=> Booting WEBrick
=> Rails 6.1.4.1 application starting in development http://localhost:3000
=> Run `bin/rails server --help` for more startup options
Exiting
passwordless/vendor/bundle/ruby/3.0.0/gems/zeitwerk-2.5.1/lib/zeitwerk/kernel.rb:35:in `require': cannot load such file -- listen (LoadError)

Therefore, use ActiveSupport::FileUpdateChecker which does not depend on listen.

config.file_watcher Rails ships with ActiveSupport::FileUpdateChecker, the default https://guides.rubyonrails.org/v6.1/configuring.html#rails-general-configuration

Test

I was able to confirm that the boot succeeded on my local.

bin/rails s
=> Booting WEBrick
=> Rails 6.1.4.1 application starting in development http://localhost:3000
=> Run `bin/rails server --help` for more startup options
[2021-12-19 18:19:45] INFO  WEBrick 1.7.0
[2021-12-19 18:19:45] INFO  ruby 3.0.0 (2020-12-25) [x86_64-darwin18]
[2021-12-19 18:19:45] INFO  WEBrick::HTTPServer#start: pid=94148 port=3000
rickychilcott commented 2 years ago

I may need a bit more context for this one. What problem were you having that caused you to make this switch?

I see https://github.com/rails/rails/issues/25186 - and I know it's related to docker, but I just don't know enough to know. Are we missing a dev dependency (listen)? Or are there any downsides to this approach?

madogiwa0124 commented 2 years ago

@rickychilcott

Thanks for the comment!

I may need a bit more context for this one. What problem were you having that caused you to make this switch?

I created this PR because I couldn't start the dev environment due to its dependency on listen.

The reason for selecting ActiveSupport::FileUpdateChecker is the intention of selecting the default value for Rails when listen is not used.

Are we missing a dev dependency (listen)?

I think adding the listen dependency is also the correct ❤️

Or are there any downsides to this approach?

There are concerns about performance degradation in the development environment, but Rails 7 defaults to using ActiveSupport::FileUpdateChecker as an acceptable one.

Modern computers with SSDs don't see much/any benefit from having an evented file update watcher. https://github.com/rails/rails/pull/42985

If it seems better to add the listen dependency, I'll close this PR and create a new PR 😃

rickychilcott commented 2 years ago

That all makes sense. No reason to add the extra dependency for minimal benefit (devs without SSDs). I'm all good on this one!

rickychilcott commented 2 years ago

Thanks so much @madogiwa0124!