feat: adding notification operator #575
Conversation
Codecov Report
@@ Coverage Diff @@
## main #575 +/- ##
=======================================
Coverage 85.39% 85.40%
=======================================
Files 765 765
Lines 15712 15719 +7
Branches 1996 1996
=======================================
+ Hits 13418 13425 +7
Misses 2261 2261
Partials 33 33
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Two ways to use this:
With #2, In case |
|
|
||
| public withNotification<T>(source: Observable<T>, successMessage: string, failureMessage: string): Observable<T> { | ||
| return source.pipe( | ||
| tap( |
There was a problem hiding this comment.
How difficult is it to make a real operator (I've done it in rxjava where it's pretty confusing, haven't looked in rxjs)? My concern with this one is that it gets awkward to use in a pipe
There was a problem hiding this comment.
Oh I take it back - it looks like rxjs operators are much simpler (should have read your other docs first). Given that, I may suggest switching the names around a bit (so the name looks more like an operator) but otherwise looks good.
something like withNotification -> wrapWithNotification and withNotificationOperator -> withNotification
There was a problem hiding this comment.
For comparison, this is what an operator in java looked like 🙄
https://github.com/hypertrace/query-service/blob/main/query-service-impl/src/main/java/org/hypertrace/core/query/service/RowChunkingOperator.java
There was a problem hiding this comment.
Yea I tried building our own operator. I got it to work too, but realized that it still has to go via notification service (since we need injected service). With that in consideration, the native implementation was not providing much value. I switched to the current approach which anyway goes via the service but is much more readable and maintainable.
There was a problem hiding this comment.
Will update the name
There was a problem hiding this comment.
Ah right - so looking at an example, pipe takes a function of UnaryFunction<Observable<T>, Observable<R>> but the standard lib ones are generally implemented via lift and an operator. I'll have to page back in if there's any benefit to using lift + operator vs pipe, but nothing seems obvious (and if so we can do it without changing the pai.
There was a problem hiding this comment.
Even their docs examples use pipe - I'd guess lift is the old way of doing it (before they used pipe when everything was on the prototype)
https://reactive.how/rxjs/pipeable-operators
There was a problem hiding this comment.
Yeah, the pipe approach is pretty popular. rxjs folks are also adding newer apis in 7.x, so this could change too.
The lift approach would require us to build our own operator which comes with additional code.
I think if we are doing something pretty involved then building our own operator makes sense. For something simple, Piggybacking on existing
operators is much cleaner fnow.
…e-ui into notification-update
This comment has been minimized.
This comment has been minimized.
Description
Please include a summary of the change, motivation and context.
feat: adding notification operator
Testing
Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.
Checklist:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.