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

Clear callbacks whenever files are changed during dev / test, to fix issue with Spring #137

Merged
merged 1 commit into from
Dec 16, 2018

Conversation

ndbroadbent
Copy link
Contributor

@ndbroadbent ndbroadbent commented Dec 2, 2018

Fixes #136.

Spring doesn't provide any hooks for reloaded files, so I used the listen gem for this. Before this, I would always have to run spring stop whenever I changed a model, because Spring would reload the models and add duplicate callbacks for stripe-rails. Everything works for me after this change, and my tests always start very quickly.

If people are not using Spring, then this change will do nothing. (I call: defined? ::Spring::Application && defined? Listen)

One very minor issue: I was able to trigger a race condition when I saved a file right when the tests started. This cleared all the callbacks after Spring had already reloaded the models, so my tests failed. But it worked when I ran the tests a second time.

@ndbroadbent
Copy link
Contributor Author

Sorry, I finally figured out the right way to do this. This was actually a problem with the code reloading in Rails development, and Spring just meant that it affected my tests as well.

The Rails Reloader provides an after_class_unload hook that I can use to clear all the callbacks before all the models are reloaded.

My initial attempt was a lot of code that set up manual file listeners, but I just needed this after_class_unload hook to clear all the callbacks before the models were reloaded.

Copy link
Owner

@tansengming tansengming left a comment

Choose a reason for hiding this comment

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

Hi @ndbroadbent thanks for looking into this! It looks like it'll make a bunch of folks happy. I just need two more things before I merge this,

  1. it looks like it's failing on Rails 4 tests, could you add a guard for this please? Thanks!
  2. how should I manually test this? Should I just update a model with a stripe callback and see if it gets updated? Any tips would be helpful.

@tansengming
Copy link
Owner

and thanks for sharing the link to the Reloader, this is a part of Rails that I didn't know about.

… files are reloaded during development and test
@ndbroadbent
Copy link
Contributor Author

Hi @tansengming, sorry I didn't see those broken tests! Should be fixed now. I couldn't see an easy way to do this in Rails 4 so I've just skipped it for now.

To test it, I've been adding a puts statement inside the after_class_unload block, and just confirming that it was called after I change a model and make a request.

I also learned quite a bit about the Rails reloading during development. When you change any file, Rails actually unloads all of the models/controllers/etc. and reloads all the files. I had assumed it was more fine-grained than that, but it just blows everything away, so it's safe to call clear_callbacks! here.

@tansengming
Copy link
Owner

I couldn't see an easy way to do this in Rails 4 so I've just skipped it for now.

No worries! Let's leave that for other developers to take a crack at 😄

I'll run a quick test later in the day before merging this. Thanks for this!

@tansengming
Copy link
Owner

I've been trying to reproduce #136 with tansengming/stripe-rails-dummy#1 without much luck.

@ndbroadbent any idea what I'm doing wrong?

This PR looks good by the way but it's curious that I'm not seeing the same issue.

@tansengming
Copy link
Owner

The testing issue was handled on tansengming/stripe-rails-dummy#1 thanks for all the help!

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