Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set up reloading after_initialize #381

Merged
merged 1 commit into from
Jun 15, 2020
Merged

Conversation

composerinteralia
Copy link
Collaborator

Alternate fix for #336. We went with #347 instead because the solution
in this commit didn't work with Rails 4.2. Since we are no longer
supporting Rails 4.2, I think this is a better approach.

The original problem looked like this:

Rails runs all of the initializers

  1. Run the "factory_bot.set_factory_paths"
    initializer
  2. Run the "factory_bot.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 Add back loading definitions in after_initialize #334

Triggering the first factory_bot reload before initializing I18n could
cause an error in some cases.

We avoided the problem in #347 by adding a conditional to skip reloading
factory_bot before the application has initialized.

This commit, on the other hand, moves factory_bot reloading from a
prepare callback into an after_initialize callback. The initialization
process is now simplified to:

Rails runs all of the initializers

  1. Run the "factory_bot.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. factory_bot loads definitions for the first time. It then runs the
    reloader to set up the prepare callback to reload factory_bot when the
    application reloads (for example by calling reload! in the console),
    and to register the reloader to trigger reloads when any factory_bot
    definition files change.

@composerinteralia composerinteralia force-pushed the i18n-loading-v2 branch 3 times, most recently from 12de522 to 2def4aa Compare May 23, 2020 15:49
Alternate fix for #336. We went with #347 instead because the solution
in this commit didn't work with Rails 4.2. Since we are no longer
supporting Rails 4.2, I think this is a better approach.

The original problem 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

Triggering the first factory_bot reload before initializing I18n could
cause an error in some cases.

We avoided the problem in #347 by adding a conditional to skip reloading
factory_bot before the application has initialized.

This commit, on the other hand, moves factory_bot reloading from a
prepare callback into an `after_initialize` callback. The initialization
process is now simplified to:

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. factory_bot loads definitions for the first time. It then runs the
reloader to set up the prepare callback to reload factory_bot when the
application reloads (for example by calling `reload!` in the console),
and to register the reloader to trigger reloads when any factory_bot
definition files change.

[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
@composerinteralia composerinteralia merged commit bfee5d8 into master Jun 15, 2020
@composerinteralia composerinteralia deleted the i18n-loading-v2 branch June 15, 2020 15:15
@dikond
Copy link

dikond commented Jun 18, 2020

In our project (legacy) we, unfortunately, have to do something like this while defining factories:

TestArticle = Class.new(Article) do
  reset_callbacks(:save)
  reset_callbacks(:create)
  reset_callbacks(:update)
  reset_callbacks(:commit)
end

FactoryBot.define do
  factory :article, class: TestArticle do
    # definitions...
  end
end

Prior to you PR we had a nasty warnings when running the spec, e.g.

/webapp/spec/factories/articles.rb:5: warning: already initialized constant TestArticle
/webapp/spec/factories/articles.rb:5: warning: previous definition of TestArticle was here

Today I decided to trace this code and find the root cause, which led me to these two hooks registered by the factory_bot_rails: run_prepare_callbacks & finisher_hook.

I was very happy to find out that you've changed this behavior just 3 days ago. I specified the latest commit SHA (bfee5d) in our Gemfile to test it out and I can confirm that this warning no longer bothers us. So thank you very much for it @composerinteralia. May I ask when do you plan to release a new version of a gem?

@composerinteralia
Copy link
Collaborator Author

composerinteralia commented Jun 18, 2020

Nice! I'm glad this change is working for you. I don't think there is much holding up a 6.0 release at this point. I might be able to get it out in the next week before going on vacation.

Warnings about redefining constants within factory definition files seem to have a way of creeping back up again and again, especially if you use spring and if you use factory_bot_rails in the development environment. My recommendation would be to move the constant into its own file somewhere in test/support or spec/support, then require that file in your factory definition file. Using require will ensure that the file only gets load once, and thus that the constant only gets loaded once (whereas factory definition files potentially get reloaded multiple times with load).

@dikond
Copy link

dikond commented Jun 18, 2020

Yeah I was also thinking about moving these test classes out of factory definitions, but it seemed like a routine task and I was much more excited to find out why it's reloaded several times (we aren't using spring).

Good that you mentioned it will be 6.0 release - I will have the time to prepare for an update if there are other breaking changes. And yeah, have a good vacation 🙂

@composerinteralia
Copy link
Collaborator Author

Released!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants