Enforce event registration #4226
Conversation
waiting-for-dev
force-pushed
the
waiting-for-dev/events_registration
branch
2 times, most recently
from
e7e56c4
to
7a8b9a6
Compare
| def register(event_name, adapter: default_adapter) | ||
| warn_registration_on_legacy_adapter if deprecation_handler.render_deprecation_message?(adapter) | ||
| return if deprecation_handler.legacy_adapter?(adapter) | ||
|
|
There was a problem hiding this comment.
Thanks! Removed.
| @@ -59,6 +59,13 @@ def adapter | |||
| order.do_something | |||
| Spree::Event.fire 'event_name', order: order | |||
|
|
|||
| - You need to register your custom events before firing or subscribing | |||
| to them (no need for Solidus' custom ones). Example: | |||
There was a problem hiding this comment.
Perhaps "not necessary for events provided by Solidus or other extensions" would be more clear.
There was a problem hiding this comment.
Fixed, I used @jarednorman's suggestion 👍
waiting-for-dev
force-pushed
the
waiting-for-dev/events_registration
branch
from
7a8b9a6
to
5aa4a21
Compare
There was a problem hiding this comment.
Thanks, Marc!
I have one note: it is not quite clear to me what is the suggested approach at registering events. Considering that people may fire events multiple times in their codebase, is the initializer the right place where people should place these Spree::Event.register calls? Maybe worth mentioning or providing an example?
| private | ||
|
|
||
| def suggestions(event_name) | ||
| dictionary = DidYouMean::SpellChecker.new(dictionary: event_names) |
Good point. I'm checking now, and I'm finding some issues because of the loading of module subscribers. Please, put it on hold until I have a clearer picture. |
Make it mandatory to register an event before being fired, subscribed
, or unsubscribed.
It helps with two scenarios:
- It avoids typo errors, like subscribing to an event `ordr_finalized`
instead of `order_finalized`.
- It avoids naming collisions between user-defined events and core ones.
Example:
```ruby
Spree::Event.register('foo')
Spree::Event.fire('foo')
```
We add this behavior only to the new, recommended adapter. However, we
add into the deprecation error for the legacy adapter the instructions
to update.
We also add a new `#unregister` method for the test interface.
waiting-for-dev
force-pushed
the
waiting-for-dev/events_registration
branch
from
5aa4a21
to
f420cba
Compare
@kennyadsl, no significant issues in the end. An event must be registered before being fired/subscribed, etc. In theory, that should mean that we can register them anywhere in the code before that. However, we have to add module subscribers into the equation. They're loaded in a That means that we must register them on an initializer. However, the initializer must run after the adapter has been configured. As Rails loads initializer in alphabetical order, one called |
|
@waiting-for-dev thanks, it works for me. |
Description
Make it mandatory to register an event before being fired, subscribed
, or unsubscribed.
It helps with two scenarios:
It avoids typo errors, like subscribing to an event
ordr_finalizedinstead of
order_finalized.It avoids naming collisions between user-defined events and core ones.
Example:
We add this behavior only to the new, recommended adapter. However, we
add into the deprecation error for the legacy adapter the instructions
to update.
We also add a new
#unregistermethod for the test interface.Checklist: