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

Fix for latest Rails 3-1-stable #11

Merged
merged 1 commit into from
Oct 9, 2011
Merged

Fix for latest Rails 3-1-stable #11

merged 1 commit into from
Oct 9, 2011

Conversation

nixme
Copy link
Contributor

@nixme nixme commented Aug 29, 2011

I noticed that while active_reload was successfully overloading the regular reload proc, classes were still being cleared after every request (ActionDispatch::Reloader). Turns out run_callbacks(:cleanup) was still executing the old proc because the memoized function wasn't cleared out. So this fix forces recompilation of ActionDispatch::Reloader's cleanup callbacks and everything works again.

Thanks for this library btw, makes a huge difference developing on Rails.

@manuelmeurer
Copy link

+1 can we get this merged in?

@paneq
Copy link
Owner

paneq commented Sep 21, 2011

Sorry that it took me so long to take care of this bug. Unfortunately I cannot reproduce it. It is even tested that after request the constant is still defined https://github.com/paneq/active_reload/blob/master/test/unit/reload_test.rb#L75 which means that code was not reloaded (by the old proc or something). Could you provide more details about what's going wrong? I added few puts statments in my rails code and do not see any difference with and without this line. However #12 issue suggests that there is something going on and I do not know what. I would be glad to here more about the bug and fix it. I tested with rails 3.1 .

@bdimcheff
Copy link

@nixme's change made active_reload work with 3.1 for me too. It definitely isn't working for me without it.

paneq added a commit that referenced this pull request Oct 9, 2011
Fix for latest Rails 3-1-stable
@paneq paneq merged commit 6844e01 into paneq:master Oct 9, 2011
@paneq
Copy link
Owner

paneq commented Oct 9, 2011

I included this fix however I am not happy that no one can provide a test case that would show that active reload works better with this fix in some situations. Maybe in the future...

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.

None yet

4 participants