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

Is Apartment::Reloader still necessary? #151

Closed
fsateler opened this issue Feb 11, 2021 · 9 comments · Fixed by #174
Closed

Is Apartment::Reloader still necessary? #151

fsateler opened this issue Feb 11, 2021 · 9 comments · Fixed by #174

Comments

@fsateler
Copy link
Contributor

The comment in railtie.rb says:

The following initializers are a workaround to the fact that I can't properly hook into the rails reloader

But this is no longer (was it ever?) true: we have (and use) to_prepare. Maybe it is no longer necessary?

@marcelolx
Copy link
Contributor

@fsateler I think the reason is better explained here https://github.com/rails-on-services/apartment/blob/development/lib/apartment/reloader.rb#L8, as I understood it, this is necessary to reload apartment only after the files are reloaded.

But I think maybe now there is a proper way to hook into the reload process after the files are reloaded, looking at the Reloader docs I think the to_complete callback would fit well for this problem 🤔 This functionality was added by this PR rails/rails#23807

@fsateler
Copy link
Contributor Author

AFAICT, to_prepare is invoked after the code has been reloaded: https://github.com/rails/rails/blob/main/activesupport/lib/active_support/reloader.rb#L58

@marcelolx
Copy link
Contributor

Sorry I miss that part 😄 Maybe this isn't really necessary anymore.

@rpbaltazar
Copy link
Contributor

so, what you're suggesting, @fsateler is to completely remove the Reloader and move the console requiring to to_prepare if in development?
That seems relatively safe for me to test in our app because it should at most affect the development environment and should be relatively simple to spot.
I'll create a branch with those changes and try it out.

@fsateler
Copy link
Contributor Author

Indeed, that's what I think. But I asked because maybe there is some weird interaction I'm not aware of...

@rpbaltazar
Copy link
Contributor

Indeed, that's what I think. But I asked because maybe there is some weird interaction I'm not aware of...

None that I know of either. I'll create the branch and run our develop env against it. Suggest you do the same on your end @fsateler and @marcelolx to see if any of our envs breaks under those conditions

@rpbaltazar
Copy link
Contributor

Opened the draft PR. Things on my end seem to be working the same way. Will run my dev env for a few days with this branch but for now looks ok

@fsateler
Copy link
Contributor Author

Thanks! will try it now

rpbaltazar added a commit that referenced this issue Feb 7, 2022
**Implemented enhancements:**

- Increase errors visibility by showing more information on the underlying error rather than a generic error 'Apartment::TenantNotFound' (#176)
- Resolved #177 - Added rails 7 support (#178)

**Fixed bugs:**

- Fixing tenant_presence_check config in the README (#180)
- Resolved #161 and #81 - Fixed sequence name (#187)

**Closed issues:**

- Resolved #151 - removed reloader and console overwrite of reload method (#174)
@otagi
Copy link

otagi commented Mar 2, 2022

With 2.11.0, the console.rb helpers are not available in the rails console anymore. They are back if I downgrade to 2.9.0.

irb(main):001:0> st
(irb):1:in `<main>': undefined local variable or method `st' for main:Object (NameError)

Is it related to this change? Should the helpers be loaded manually now?

(ruby 3.0.3, rails 6.1.4.6)

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 a pull request may close this issue.

4 participants