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.

Sign up for GitHub

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

Jump to bottom

[Events] Add subscribers automatically #3571

Merged
merged 3 commits into from
Jun 26, 2020
Merged

[Events] Add subscribers automatically #3571

merged 3 commits into from
Jun 26, 2020

Conversation

spaghetticode
Copy link
Member

Description

(This PR is based on some work done by @elia and @AlessioRocco for Meundies)

Until now, adding subscribers to Spree::Event.subscribers was developers' responsibility.

This PR aims at automating the task by making Rails load subscriber files located in standard paths both in Solidus core and in the store app during the Rails initialization processs. When each file is required, it's now also added to the Spree::Event.subscribers list.

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)
  • I have attached screenshots to this PR for visual changes (if needed)

@spaghetticode spaghetticode self-assigned this Mar 30, 2020
houndci-bot
houndci-bot reviewed Mar 30, 2020
View reviewed changes
core/spec/lib/spree/event_spec.rb Outdated Show resolved Hide resolved
core/lib/spree/event.rb Outdated Show resolved Hide resolved
core/lib/spree/event.rb Show resolved Hide resolved
core/lib/spree/event.rb Outdated Show resolved Hide resolved
@spaghetticode spaghetticode force-pushed the spaghetticode/reload-subscribers branch 3 times, most recently from 59b2c11 to 60f7746 Compare March 30, 2020 14:40
@spaghetticode spaghetticode marked this pull request as ready for review March 31, 2020 09:05
@spaghetticode spaghetticode force-pushed the spaghetticode/reload-subscribers branch from 60f7746 to 60e4b0f Compare April 10, 2020 07:39
@kennyadsl
Copy link
Member

A couple of questions:

  1. What about extensions? Do they need to register things manually?
  2. What if someone already registered an event manually in their store? Does this end up with two subscriptions?

@kennyadsl kennyadsl changed the title Spaghetticode/reload subscribers [Events] Add subscribers automatically Apr 17, 2020
@spaghetticode
Copy link
Member Author

spaghetticode commented May 8, 2020 •
edited
Loading

@kennyadsl here you are my answers to your questions:

  1. at this time extensions still need to register their events manually. I suppose we can achieve that by modifying solidus_support

  2. event subscribers are subscribed only once, so doing it manually as well does not create any problem. This is how Spree::Event::Subscriber already works, see this spec

kennyadsl
kennyadsl approved these changes May 8, 2020
View reviewed changes
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Thanks, loading core subscribers automatically works for me. I have some concerns just in case we may want to add extra logic in the future, for example, deprecating a subscriber and avoiding it to be always loaded, but we can face it when/if this will happen.

@aldesantis
Copy link
Member

aldesantis commented May 8, 2020 •
edited
Loading

I'm not 100% sure about this to be honest, I think it adds a layer of magic that we may not need. It could be particularly confusing considering that core subscribers are loaded automatically, while extensions will have to register their subscribers manually. At the end of the day, is it really worth it? 🤔

jarednorman
jarednorman reviewed May 8, 2020
View reviewed changes
Copy link
Member

@jarednorman jarednorman left a comment

Choose a reason for hiding this comment

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

My only concern here would be making sure this all works properly with code reloading.

@spaghetticode
Copy link
Member Author

@jarednorman I just checked, code reload still works fine in development (kudos go to @elia for the implementation).

@aldesantis
Copy link
Member

We discussed this during a core team meeting and, since the plan is to add the same capability to extensions, I'm happy to move forward with this! 🚀 Thanks @spaghetticode and @elia.

@aldesantis aldesantis added release:major Breaking change on hold until next major release Needs Work and removed release:major Breaking change on hold until next major release labels May 22, 2020
@jarednorman
Copy link
Member

Cool. I'll hold off giving this an approval since it's still listed as "Needs Work" but I'm favour of this change overall.

@spaghetticode spaghetticode force-pushed the spaghetticode/reload-subscribers branch 2 times, most recently from de5ba6e to adcf05d Compare May 29, 2020 10:11
This was referenced May 29, 2020
Merged
Closed
@spaghetticode
Copy link
Member Author

This functionality is made available on extensions by this PR from solidus_support.

Also, if anybody is interested in giving this feature a try I created a demo app that quickly showcases event subscribers autoload from in Solidus core, Solidus extensions and the rails app itself.

@aldesantis
Copy link
Member

@spaghetticode looks like f8f392a somehow ended up in this branch 😄 Can you remove it?

@spaghetticode
Add preference for autoloading subscribers
3d4fbf3 
The preference `Spree::Config.event.autoload_subscribers` can be used
for disabling the automatic loading of Solidus event subscribers.

The preference is set to `true` by default, developers can opt out by
creating an intializers that sets the preference to false:

  Spree::Config.event.autoload_subscribers = false
@spaghetticode
Dynamically (re)load event subscribers
e5a6c08 
Until now, adding subscribers to Spree::Event.subscribers was
developers' responsibility.

This commit aims at automating the task by making Rails load
subscriber files located in standard paths both in Solidus core
and in the store app during the Rails initialization processs.

When each file is required, it's now also added to the
`Spree::Event.subscribers` list.

Solidus core event subscribers are always loaded automatically,
while the ones included in the store app can be loaded manually
when the preference Spree::Config.events.autoload_subscribers
is changed to a falsey value.

A similar feature will be added to `solidus_support` in order
to automatically subscribe subscribers from extensions.
@spaghetticode
Update Event guides
d617740 
Documenting automatic loading of files and subscription of subscribers
when their files are placed under the directory `app/subscribers` and
end with `_subscriber.rb`.
@spaghetticode spaghetticode force-pushed the spaghetticode/reload-subscribers branch from adcf05d to d617740 Compare June 26, 2020 12:31
@spaghetticode
Copy link
Member Author

@aldesantis 👍

@aldesantis
Copy link
Member

@spaghetticode GTM! :shipit:

@spaghetticode spaghetticode merged commit 08f6301 into solidusio:master Jun 26, 2020
@spaghetticode spaghetticode deleted the spaghetticode/reload-subscribers branch June 26, 2020 15:35
@spaghetticode spaghetticode mentioned this pull request Jul 31, 2020
Closed
@spaghetticode spaghetticode mentioned this pull request Sep 11, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@jarednorman jarednorman jarednorman left review comments

@AlessioRocco AlessioRocco AlessioRocco left review comments

@aldesantis aldesantis aldesantis approved these changes

@kennyadsl kennyadsl kennyadsl approved these changes

Assignees

@spaghetticode spaghetticode

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@spaghetticode @kennyadsl @aldesantis @jarednorman @AlessioRocco @houndci-bot