From 35bb9409aa9b42155e643a4f4c4182ada640c428 Mon Sep 17 00:00:00 2001 From: Daniel Colson Date: Fri, 26 Jul 2019 17:38:00 -0400 Subject: [PATCH] Set up reloading after_initialize Fixes #336 Before this commit, the initialization process looked like this: Rails runs all of the initializers 1. Run the ["factory_bot.set_factory_paths"][set_factory_paths] initializer 2. Run the ["factory_bot.register_reloader"][register_reloader] initializer, which sets up a [prepare callback][] 3. Run the [`:run_prepare_callbacks`][] initializer 4. This triggers the factory_bot [prepare callback][], which causes factory\_bot to [reload][] Rails runs `after_initialize` callbacks 1. [I18n initializes] 2. factory\_bot [reloads again][] as described in #334 The double reloading of factory_bot in this initialization is not ideal, but also shouldn't generally cause any problems on its own. The problems people are having in #336 come from the fact that I18n gets set up in an `after_initialize` callback, but factory_bot gets reloaded before the `after_initialize` callbacks are triggered. If the `FactoryBot.define` block references any code that uses I18n translations as it loads, that code will raise an error (references inside other factory_bot methods, or code that uses I18n translations inside of methods still works fine, since the whole Rails initialization process would be complete by the time any of that code runs). This commit moves factory_bot reloading from a prepare callback into an `after_initialize` callback. This allows us to avoid reloading factory_bot before I18n is loaded, and also gets rid of that pesky double reloading of factory_bot. The new initialization process looks like: Rails runs all of the initializers 1. Run the ["factory_bot.set_factory_paths"][set_factory_paths] 2. Run the [`:run_prepare_callbacks`][] initializer, which no longer involves factory_bot Rails runs `after_intialize` callbacks 1. [I18n initializes] 2. Run the factory_bot reloader, which sets up the prepare callback for any future prepares (for example calling `reload!` in the console), and executes the reloader to run the initial `FactoryBot.reload` [set_factory_paths]: https://github.com/thoughtbot/factory_bot_rails/blob/3815aae2b9e4a5c5c3027a2ee8851f44b7c6a4da/lib/factory_bot_rails/railtie.rb#L17-L19 [register_reloader]: https://github.com/thoughtbot/factory_bot_rails/blob/3815aae2b9e4a5c5c3027a2ee8851f44b7c6a4da/lib/factory_bot_rails/railtie.rb#L21-L23 [prepare callback]: https://github.com/thoughtbot/factory_bot_rails/blob/master/lib/factory_bot_rails/reloader.rb#L34-L36 [`:run_prepare_callbacks`]: https://github.com/rails/rails/blob/5-2-stable/railties/lib/rails/application/finisher.rb#L62-L64 [reload]: https://github.com/thoughtbot/factory_bot_rails/blob/master/lib/factory_bot_rails/reloader.rb#L24-L26 [I18n initializes]: https://github.com/rails/rails/blob/13e2102517fafc8f8736fce5d57de901067202d0/activesupport/lib/active_support/i18n_railtie.rb#L16-L20 [reloads again]: https://github.com/thoughtbot/factory_bot_rails/blob/master/lib/factory_bot_rails/railtie.rb#L25-L27 --- lib/factory_bot_rails/railtie.rb | 8 ++------ lib/factory_bot_rails/reloader.rb | 15 ++++++++------- spec/factory_bot_rails/reloader_spec.rb | 24 +++++++++++++++--------- 3 files changed, 25 insertions(+), 22 deletions(-) diff --git a/lib/factory_bot_rails/railtie.rb b/lib/factory_bot_rails/railtie.rb index 2e9cd81..c8669a7 100644 --- a/lib/factory_bot_rails/railtie.rb +++ b/lib/factory_bot_rails/railtie.rb @@ -18,12 +18,8 @@ module FactoryBotRails FactoryBot.definition_file_paths = definition_file_paths end - initializer "factory_bot.register_reloader" do |app| - Reloader.new(app, config).run - end - - config.after_initialize do - FactoryBot.reload + config.after_initialize do |app| + Reloader.new(app).run end private diff --git a/lib/factory_bot_rails/reloader.rb b/lib/factory_bot_rails/reloader.rb index f3d66fd..e54fde1 100644 --- a/lib/factory_bot_rails/reloader.rb +++ b/lib/factory_bot_rails/reloader.rb @@ -4,21 +4,22 @@ require "factory_bot_rails/definition_file_paths" module FactoryBotRails class Reloader - def initialize(app, config) + def initialize(app) @app = app - @config = config @paths = DefinitionFilePaths.new(FactoryBot.definition_file_paths) end def run return unless @paths.any? - register_reloader(build_reloader) + reloader = build_reloader + register_reloader(reloader) + reloader.execute end private - attr_reader :app, :config + attr_reader :app def build_reloader reloader_class.new(@paths.files, @paths.directories) do @@ -31,11 +32,11 @@ module FactoryBotRails end def register_reloader(reloader) - config.to_prepare do + app.reloaders << reloader + + app.reloader.to_prepare do reloader.execute end - - app.reloaders << reloader end end end diff --git a/spec/factory_bot_rails/reloader_spec.rb b/spec/factory_bot_rails/reloader_spec.rb index 188e791..d66d217 100644 --- a/spec/factory_bot_rails/reloader_spec.rb +++ b/spec/factory_bot_rails/reloader_spec.rb @@ -12,8 +12,6 @@ describe FactoryBotRails::Reloader do context "when a definition file paths exist" do it "registers a reloader" do - allow(reloader_class).to receive(:new) - run_reloader(["spec/fixtures/factories", "not_exist_directory"]) expect(reloader_class).to have_received(:new) @@ -22,8 +20,6 @@ describe FactoryBotRails::Reloader do context "when a file exists but not a directory" do it "registers a reloader" do - allow(reloader_class).to receive(:new) - run_reloader(["spec/fake_app", "not_exist_directory"]) expect(reloader_class).to have_received(:new) @@ -32,8 +28,6 @@ describe FactoryBotRails::Reloader do context "when a definition file paths NOT exist" do it "does NOT register a reloader" do - allow(reloader_class).to receive(:new) - run_reloader(["not_exist_directory"]) expect(reloader_class).not_to have_received(:new) @@ -42,12 +36,24 @@ describe FactoryBotRails::Reloader do def run_reloader(definition_file_paths) FactoryBot.definition_file_paths = definition_file_paths - FactoryBotRails::Reloader. - new(Rails.application, Rails.application.config).run + FactoryBotRails::Reloader.new(app).run + end + + def app + double( + :app, + config: double(:config, file_watcher: reloader_class), + reloader: double(:reloader, to_prepare: nil), + reloaders: [], + ) end def reloader_class - Rails.application.config.file_watcher + @reloader_class ||= + double( + :reloader_class, + new: double(:reloader, execute: nil), + ) end end end