-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Use Omnes for pub/sub #4342
Use Omnes for pub/sub #4342
Conversation
cc @forkata |
bb80bcb
to
c4c34b4
Compare
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.
Nicely implemented, thanks @waiting-for-dev.
🚌 🚌 🚌
core/lib/spree/bus.rb
Outdated
Bus = Omnes::Bus.new.tap do |bus| | ||
bus.define_singleton_method(:publish) do |*args, **kwargs, &block| | ||
if Spree::Config.use_legacy_events | ||
Spree::Event.fire(*args, **kwargs, &block) | ||
else | ||
super(*args, **kwargs, caller_location: caller_locations(1)[0], &block) | ||
end | ||
end | ||
end |
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.
Not a strong opinion, but I know tap
can be a stumbling block for people not familiar with it. I like tap
, but only use it when it actually makes the code cleaner or avoids polluting a scope with too many local variables. The use here doesn't really seem to add much to me, since we could write it like this instead and I think it's just as good, but doesn't require you to be familiar with tap
:
Bus = Omnes::Bus.new.tap do |bus| | |
bus.define_singleton_method(:publish) do |*args, **kwargs, &block| | |
if Spree::Config.use_legacy_events | |
Spree::Event.fire(*args, **kwargs, &block) | |
else | |
super(*args, **kwargs, caller_location: caller_locations(1)[0], &block) | |
end | |
end | |
end | |
Bus = Omnes::Bus.new | |
Bus.define_singleton_method(:publish) do |*args, **kwargs, &block| | |
if Spree::Config.use_legacy_events | |
Spree::Event.fire(*args, **kwargs, &block) | |
else | |
super(*args, **kwargs, caller_location: caller_locations(1)[0], &block) | |
end | |
end |
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.
Fair enough. Changed. Thanks, @jarednorman.
c4c34b4
to
441e651
Compare
Updated with a fix for autoloading subscribers (see the addition of |
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.
Nice work here @waiting-for-dev!
if Spree::Config.use_legacy_events | ||
Spree::Event.fire(*args, **kwargs, &block) | ||
else | ||
super(*args, **kwargs, caller_location: caller_locations(1)[0], &block) |
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.
I was a bit puzzled as to why we are overriding the caller_location
here. Would it be worth leaving a comment that this is intended to omit this from the stack trace, or just dropping that in order to ease debugging for anyone who is getting the new bus implementation and trying to track down why?
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.
I went with the addition of a comment. We do need to go one level higher in the stack. Otherwise, it'll be useless for debugging, as all events will look as published from this line (see https://github.com/nebulab/omnes/blob/bb73fce75414816e304a7402efbea655b82c857b/lib/omnes/bus.rb#L187)
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.
Thanks for clarifying this @waiting-for-dev, your decision here makes a lot more sense now.
441e651
to
9ed98fe
Compare
if Spree::Config.use_legacy_events | ||
Spree::Event.fire(*args, **kwargs, &block) | ||
else | ||
super(*args, **kwargs, caller_location: caller_locations(1)[0], &block) |
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.
Thanks for clarifying this @waiting-for-dev, your decision here makes a lot more sense now.
end | ||
else | ||
app.reloader.to_prepare do | ||
Spree::Bus.clear |
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 this change anything with regards to where applications or Solidus extensions should be registering any custom events they want to introduce that use the Spree::Bus
🤔
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.
That's a great question. More than a change, it's a fix. To survive code reloading, events need to be registered, and subscriptions must be added within a #to_prepare
block (which runs one on production but at every reload on development). Solidus owns Spree::Bus
, so it clears it at every reload. Then, solidus itself, extensions, and applications can just call #register
& #subscribe_to
within their own #to_prepare
blocks, which will run later in time.
Rebasing from master, and updating Omnes version to 0.2.2 to support Ruby 2.5 & Ruby 2.6, which are still supported on Solidus. |
9ed98fe
to
6f9cea7
Compare
This looks good to me, thanks Marc for the usual great work! What about removing the spurious fixup commit that updates |
Thanks, @spaghetticode. Oh, sorry, I missed that (forgot the rebase). I'll fix it 🙂 |
Thanks for spotting it, @spaghetticode. Fixed! |
I still see the fixup commit: cb0e910 To be super explicit, here's the three commits related to the omnes dependency: |
6f9cea7
to
5873295
Compare
My bad @spaghetticode. I forced the push to the wrong repo. Everything should be fine now. |
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.
thank you Marc!
We'll use [omnes](https://github.com/nebulab/omnes) as a pub/sub library, so the first step is reverting what we had done. Omnes is an extraction of that work plus other new features. Reverted commits: - e1662c2 - d4d3c0c - 7d9b4e9 - f420cba - 8d73d23 - c22270b - 6a95866 - 98b8214 - 620298a - d50fc68
Using Omnes will be the default way to make usage of pub/sub features from Solidus 3.2 onwards. This commit prepares the stage by adding it as a dependency. A new `Spree::Config#use_legacy_events` preference has been added to allow using the old system based on `ActiveSupport::Notifications`. We still keep it as the default system because internal subscribers still only work with it.
We add an Omnes subscriber class for order mailing work, equivalent to the legacy subscriber module. From now on, new applications will use Omnes by default. We also run our test suite with it, but we still allow running with the legacy adapter through a `USE_LEGACY_EVENTS` environment variable (and use it in a new CI job).
Spree::Bus (using Omnes under the hood) is the new recommended way for pub/sub on Solidus. We update the test helpers to work with it. We update naming to use `bus` instead of `event`. Omnes is more restrictive and requires event names to be a Symbol.
Once we've the upgrade guide complete, we'll update the message with a link to it.
5873295
to
3f3e84d
Compare
Rebased from master and made a minor change to adopt the new way to define versioned preferences (see #4375). |
See #4380 |
The specs here were specific to the legacy implementation of the event system, and therefore no longer pass on the `master` branch of Solidus where we now use a [new implementation of the event bus](solidusio/solidus#4342). This change attempts to refactor the tests to focus on observing the side-effects of the object under test which should be independent of the underlying implementation. This test is valuable regardless of what version of Solidus we are running against as our extension relies on the event being published when a specific action happens and that the payload contains the necessary object. Co-authored-by: Andrew Stewart <andrew@super.gd>
The specs here were specific to the legacy implementation of the event system, and therefore no longer pass on the `master` branch of Solidus where we now use a [new implementation of the event bus](solidusio/solidus#4342). This change attempts to refactor the tests to focus on observing the side-effects of the object under test which should be independent of the underlying implementation. This test is valuable regardless of what version of Solidus we are running against as our extension relies on the event being published when a specific action happens and that the payload contains the necessary object. Co-authored-by: Andrew Stewart <andrew@super.gd>
The specs here were specific to the legacy implementation of the event system, and therefore no longer pass on the `master` branch of Solidus where we now use a [new implementation of the event bus](solidusio/solidus#4342). This change attempts to refactor the tests to focus on observing the side-effects of the object under test which should be independent of the underlying implementation. This test is valuable regardless of what version of Solidus we are running against as our extension relies on the event being published when a specific action happens and that the payload contains the necessary object. Co-authored-by: Andrew Stewart <andrew@super.gd>
The specs here were specific to the legacy implementation of the event system, and therefore no longer pass on the `master` branch of Solidus where we now use a [new implementation of the event bus](solidusio/solidus#4342). This change attempts to refactor the tests to focus on observing the side-effects of the object under test which should be independent of the underlying implementation. This test is valuable regardless of what version of Solidus we are running against as our extension relies on the event being published when a specific action happens and that the payload contains the necessary object. Co-authored-by: Andrew Stewart <andrew@super.gd>
The specs here were specific to the legacy implementation of the event system, and therefore no longer pass on the `master` branch of Solidus where we now use a [new implementation of the event bus](solidusio/solidus#4342). This change attempts to refactor the tests to focus on observing the side-effects of the object under test which should be independent of the underlying implementation. This test is valuable regardless of what version of Solidus we are running against as our extension relies on the event being published when a specific action happens and that the payload contains the necessary object. Co-authored-by: Andrew Stewart <andrew@super.gd>
The specs here were specific to the legacy implementation of the event system, and therefore no longer pass on the `master` branch of Solidus where we now use a [new implementation of the event bus](solidusio/solidus#4342). This change attempts to refactor the tests to focus on observing the side-effects of the object under test which should be independent of the underlying implementation. This test is valuable regardless of what version of Solidus we are running against as our extension relies on the event being published when a specific action happens and that the payload contains the necessary object. Co-authored-by: Andrew Stewart <andrew@super.gd>
The specs here were specific to the legacy implementation of the event system, and therefore no longer pass on the `master` branch of Solidus where we now use a [new implementation of the event bus](solidusio/solidus#4342). This change attempts to refactor the tests to focus on observing the side-effects of the object under test which should be independent of the underlying implementation. This test is valuable regardless of what version of Solidus we are running against as our extension relies on the event being published when a specific action happens and that the payload contains the necessary object. Co-authored-by: Andrew Stewart <andrew@super.gd>
The specs here were specific to the legacy implementation of the event system, and therefore no longer pass on the `master` branch of Solidus where we now use a [new implementation of the event bus](solidusio/solidus#4342). This change attempts to refactor the tests to focus on observing the side-effects of the object under test which should be independent of the underlying implementation. This test is valuable regardless of what version of Solidus we are running against as our extension relies on the event being published when a specific action happens and that the payload contains the necessary object. Co-authored-by: Andrew Stewart <andrew@super.gd>
The specs here were specific to the legacy implementation of the event system, and therefore no longer pass on the `master` branch of Solidus where we now use a [new implementation of the event bus](solidusio/solidus#4342). This change attempts to refactor the tests to focus on observing the side-effects of the object under test which should be independent of the underlying implementation. This test is valuable regardless of what version of Solidus we are running against as our extension relies on the event being published when a specific action happens and that the payload contains the necessary object. Co-authored-by: Andrew Stewart <andrew@super.gd>
Description
This PR removes the new event bus adapter and delegates the pub/sub backbone to an external dependency: omnes. Omnes is, in fact, an extraction of that code into a standalone library, which has given us the possibility to add more features from which Solidus can benefit.
The changes can be more easily reviewed commit by commit:
Spree::Bus
instance. That is something that won't be on Omnes, as it's a specific stub for our global bus instance.How will Solidus benefit from it?
Being free from the need to support two completely different adapters through the same API enormously simplifies our code. We've also been able to add new features without constraints. Omnes has better support for debugging, testing, and async (see its README for details). We can potentially benefit from contributors using it on other kinds of projects.
There's still a place to take more advantage of omnes in future work:
How will people upgrade from the legacy system to omnes?
As pointed out, we'll write an upgrade guide and add a link to it in the deprecation message. As heads-up:
Spree::Event
toSpree::Bus
.#fire
to#publish
.#publish
. Instead, run whatever you want before it.#subscribe_to_all
. If they really want it, they can still create a custom matcher on omnes.How will people upgrade from master to omnes?
All the work on the new event bus adapter was on master, but it hadn't been released yet.
Anyway, the migration from master will be pretty straightforward. We'll happily support it, and once this gets merged, we'll post a message on Slack to let people know they can come to us (more explicitly to @waiting-for-dev) if they have any issues.
Checklist: