-
Notifications
You must be signed in to change notification settings - Fork 28
Description
I have a couple concerns with the instrumentation implementation presently in use.
-
Currently, the Project's
notification_center
is being type-checked against Optimizely::Notificiation center. This anti-pattern violates one of ruby's core idioms of duck typing. A notification center should only need to conform to the expected API. In particular, as long as the given object responds tosend_notifications
, then it should be considered a valid notification center. -
Secondly, the existing NotificationCenter api does not conform to semi-standard notification APIs in Rails land. A de-facto standard API in Rails land is
ActiveSupport::Notifications
, of which there are many implementations. Conforming to this API would allow existing instrumentation objects within a codebase to be passed to the Optimizely project to handle the new optimizely events. -
Lastly, the existing NotificationCenter only allows registration of(addressed by NotificationCenter should accept any Callable #216 🎉 )Method
listeners. This is completely abnormal ruby, as a normal ruby listener could be a block, proc, or lambda. Even better, it should be anything that responds tocall
, but at the very least, accepting block/proc/lambdas is table-stakes.