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 #343

Closed
wants to merge 1 commit into from

Conversation

composerinteralia
Copy link
Collaborator

@composerinteralia composerinteralia commented Jul 26, 2019

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" 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

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"
  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

@composerinteralia
Copy link
Collaborator Author

composerinteralia commented Jul 26, 2019

I would like to get a failing test before merging this, but I had trouble making that happen.

!@y-yagi it would be great if you could take a look at this PR, if you have a moment. I seem to be playing Whac-A-Mole with the bugs related to this reloading feature. Failing a bunch of specs on CI. I will ping you again once I get all of that resolved.

@composerinteralia composerinteralia force-pushed the i18n-loading-bug branch 2 times, most recently from c1e881d to a100f62 Compare July 26, 2019 21:38
composerinteralia added a commit that referenced this pull request Sep 20, 2019
Fixes #336
Alternate solution to #343

The initialization process looks 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 changes step 4 above to avoid reloading factory_bot before
the application has initialized.

[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

Co-authored-by: Danny Garcia <[email protected]>
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
composerinteralia added a commit that referenced this pull request Sep 20, 2019
Fixes #336
Alternate solution to #343

The initialization process looks 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 changes step 4 above to avoid reloading factory_bot before
the application has initialized.

[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

Co-authored-by: Danny Garcia <[email protected]>
Copy link
Contributor

@aledustet aledustet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a more elegant solution that doesn't relly on double reloading, wonder if a test like the one on #347 would make sense?

@@ -4,21 +4,22 @@

module FactoryBotRails
class Reloader
def initialize(app, config)
def initialize(app)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything we should include in the docs now there is not a config to be passed in?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is not meant for public use, so changing the API should be fine.

config.to_prepare do
app.reloaders << reloader

app.reloader.to_prepare do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way I understand this, the to_prepare block that was part of the config before now will be under the reloaders?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In later versions of Rails this amounts to the same thing. config.to_prepare pushes the callbacks into an array which later gets copied over to app.reloader.to_prepare.

But it didn't work quite like this in Rails 4.2, which is why we are getting CI failures.

@composerinteralia
Copy link
Collaborator Author

I am going to close this for now. When we drop support for Rails 4.2 we can revisit this, and I agree we will definitely want to keep the test from #347.

composerinteralia added a commit that referenced this pull request Oct 2, 2019
Fixes #336
Alternate solution to #343

The initialization process looks 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 changes step 4 above to avoid reloading factory_bot before
the application has initialized.

[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

Co-authored-by: Danny Garcia <[email protected]>
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.

Issues with custom error message translations
2 participants