-
-
Notifications
You must be signed in to change notification settings - Fork 23
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
Compatibility layer for the legacy/new event system #70
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@waiting-for-dev I have a question, but I'm going to approve anyway, because I'm pretty sure it's just me not being familiar enough with Omnes.
legacy_subscriber.define_singleton_method(:omnes_subscriber) do | ||
@omnes_subscriber ||= Class.new.include(::Omnes::Subscriber).tap do |subscriber| | ||
legacy_subscriber.event_actions.each do |(legacy_subscriber_method, event_name)| | ||
subscriber.handle(event_name.to_sym, with: ADAPTER.curry[legacy_subscriber, legacy_subscriber_method]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Omnes pass the omnes_subscriber
as the first argument? It seems like it either passes the event
or the event
and the publication_context
, but I might be mis-interpreting what's going on here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. The subscription callback takes what you said. However, what with:
needs to return is a callable taking the omnes subscriber as the first argument, while the following two arguments do match subscription callback expectations. That way, the adapters can do their job with the first argument and return a regular subscription. See https://github.com/nebulab/omnes#custom-adapters for details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a question about the requires order but ok to merge anytime.
@@ -3,6 +3,7 @@ | |||
require 'solidus_support/version' | |||
require 'solidus_support/migration' | |||
require 'solidus_support/engine_extensions' | |||
require 'solidus_support/legacy_event_compat' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using SolidusSupport::LegacyEventCompat
in lib/solidus_support/engine_extensions
. Wouldn't be more correct to require this before that file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! Done.
Adds a `SolidusSupport::LegacyCompat` module to provide helpers for extensions that need to support both the legacy event system (`Spree::Event`) and the new one (`Spree::Bus`).
e3a960b
to
94845b8
Compare
Adds a
SolidusSupport::LegacyCompat
module to provide helpers forextensions that need to support both the legacy event system
(
Spree::Event
) and the new one (Spree::Bus
).