Support class reloading for Event Subscribers

[elia]

Description

Before this change, editing an instance of Spree::Event::Subscriber would
result in not being reloaded or being subscribed twice.

With this change each includer of Event::Subscriber is registered by its name
and later unsubscribed just before class unload (using old module instance),
then resubscribed after the new code is loaded.

Credit: This work has been done together with the estimable @AlessioRocco

Checklist:

• I have followed Pull Request guidelines
• I have added a detailed description into each commit message
• I have updated Guides and README accordingly to this change (if needed)
• I have added tests to cover this change (if needed)

[kennyadsl]

Thanks, Left some initial change requests and questions

[tvdeyen]

This is still WIP, but before we put too much affort into this:

Is there any other way of fixing these (Rails.env.development only related) problems?

I really don't like to add this amount of code into every store that gets created out there. Could you elaborate why this change is necessary and maybe discuss another way of fixing those issues?

From what I can tell in the moment this looks like an implemenation issue we have with the current subscribers implementation, right? If so, could we try to fix those instead?

Thanks 🌹

[elia]

@tvdeyen thanks for the early heads up!

---

I really don't like to add this amount of code into every store that gets created out there. Could you elaborate why this change is necessary and maybe discuss another way of fixing those issues?

I'll defer to the section below for explaining how this is necessary for the current subscriber implementation (AS::Notifications) and most probably for alternative ones. I also agree that keeping that amount of code there is not ideal. What about leaving just the glob and subscribe with an internal API? E.g.:

Dir.glob(Rails.root.join('app/subscribers/*_subcriber.rb)) do |path|
  Spree::Event.register_subscriber(path)
end

Alternatively we can also define the glob in a configuration, e.g. Spree::Config.events.files = Dir.glob(Rails.root.join('app/subscribers/*_subcriber.rb)), and do everything else internally.

---

From what I can tell in the moment this looks like an implemenation issue we have with the current subscribers implementation, right? If so, could we try to fix those instead?

That's correct, namely the problem is with AS::Notifications, although, that's likely something that will be needed for any other subscriber implementation that organizes subscribers in different classes/modules across different files (which seems a pretty standard way of doing it). And allowing an implementation to provide a list of constant-names to be re-subscribed at each reload seems a general-enough api.

To better exemplify the problem I'll write some psedo-code that will hopefully make it clear for anyone not super-familiar with rails reloading, the following is based on the current implementation / examples.

I apologize if this is known stuff, but I thought it might be a useful refresher for some 🙂

Scenario 1: loading subscribers with an initializer

The app starts and subscribes all subscribers

# config/initializers/spree_event_subscribers.rb
OrderSubscriber.subscribe! # Rails will load app/subscribers/order_subscriber.rb
PaymentSubscriber.subscribe! # Rails will load app/subscribers/payment_subscriber.rb

Rails will load the missing constants in this way

unless const_defined? :OrderSubscriber
  load 'app/subscribers/order_subscriber.rb'
  mark_as_autoloaded :OrderSubscriber, 'app/subscribers/order_subscriber.rb'
  p OrderSubscriber # => #<Module:123>
end

AS::Notification will store the contents of OrderSubscriber inside a global list of subscribers

AS::Notification.subscribers << OrderSubscriber # that's an instance of Module: #<Module:123>

The developer changes the contents of app/subscribers/order_subscriber.rb

if file_changed? 'app/subscribers/order_subscriber.rb'
  remove_const :OrderSubscriber
end

# at this point AS::Notification still holds a reference to the old version of OrderSubscriber  
p AS::Notification.subscribers # => [#<Module:123>, …]

The new version of OrderSubscriber is never loaded since the constant is not referenced anywhere except in the initializer

Scenario 2: loading subscribers with to_prepare

Goes like scenario 1, but order_subscriber.rb is loaded within the to_prepare block which is called at initialization and after each application reload

When the app reloads the constant is removed as above, and redefined with the new implementation of OrderSubscriber, let's call it #Module:456, which is a completely different instance of Module

The problem remains with the global AS::Notifications global list that still holds the old reference, plus the new reference: [#<Module:123>, …, #<Module:456>, …]

The result in this case is that we have duplicate subscribers with different implementations

[kennyadsl]

Thanks, @elia. I think that this PR is way better now. I left a comment about moving some changes into another PR, let me know!

[kennyadsl]

I'm ok with keeping these changes (both commits for this file) but maybe now it's better to move them into a separate PR. They don't seem to be related to this one anymore, right?

[elia]

Done ✅

[kennyadsl]

@elia does this apply only to app/subscribers of Solidus core or it will add to the path also subscribers defined into extensions and host applications?

[elia]

@kennyadsl good call, after reading a bunch of rails code and some trial and error I verified I misinterpreted how paths worked. I'll push an update that fixes the behavior for both engine and app code.

[kennyadsl]

We are discussing this IRL and we decided to try to use an approach similar to calculators so we'll probably end up having a list of subscribers classes into a preference like

spree.config.event.subscribers = [
  Spree::Core::Event::MailSubscriber,
  SpreeExtensions::AnotherSubscriber,
  MyApp::YetAnotherSubscriber
]

This looks like a good solution for a couple of reasons:

1. it is something easy to manage to add change or remove classes in the list
2. it is a well-known pattern for Solidus users since we are using it in other places. If we find a more Rails-way solution for this kind of object we can easily replace everything and deprecate setting the configuration.

[elia]

@kennyadsl @tvdeyen ready for a new (final?) pass 🔍🙂

[kennyadsl]

👍Now it looks perfect to me. Thanks, @elia!

[tvdeyen]

Awesome work, Elia! Thanks for rethinking the first approach and implementing the alternative, what I consider a great solution to that problem 🍨