-
Notifications
You must be signed in to change notification settings - Fork 83
feat(event processor): Send log event notification upon event dispatch when notification center available #339
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
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.
lgtm, please rebase PR so we don't get changes from other PRs mixed in
@@ -38,7 +38,7 @@ | |||
}, | |||
"dependencies": { | |||
"@optimizely/js-sdk-logging": "^0.1.0", | |||
"@optimizely/js-sdk-utils": "^0.1.0" | |||
"@optimizely/js-sdk-utils": "^0.2.0" |
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 think these changes are not supposed to be in this PR
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 did intend this. I'm updating to 0.2.0 so I can get access to NOTIFICATION_TYPES
and NotificationCenter
, new exports from js-sdk-utils. But did you want this update itself to be separated into its own PR?
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.
Lgtm
Summary
With this change, the event processor can be constructed with a notification center. If a notification center is provided, it triggers a log event notification after the event is sent to the event dispatcher.
Test plan
Added new unit test. Manually tested
Issues
https://optimizely.atlassian.net/browse/OASIS-4976