-
Notifications
You must be signed in to change notification settings - Fork 8.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
[ui/utils] share sync subscribe logic #23341
[ui/utils] share sync subscribe logic #23341
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
💚 Build Succeeded |
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.
src/ui/public/utils/subscribe.ts
Outdated
$scope.$apply(() => fn(...args)); | ||
} | ||
} catch (error) { | ||
fatalError(error); |
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.
question: not sure if it's a big deal, but shouldn't we unsubscribe from original observable if we call fatalError
when next
fails?
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.
Rxjs takes care of that because fatalError()
rethrows the error and when a next()
handler throws the parent is unsubscribed.
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.
Ah, right, I forgot that fatalError
always throws.
src/ui/public/utils/subscribe.ts
Outdated
* Subscribe to an observable at a $scope, ensuring that the digest cycle | ||
* is run for subscriber hooks and routing errors to fatalError if not handled. | ||
*/ | ||
export function subscribe<T>( |
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.
note: subscribe
is a bit too generic, but I don't know a better name either, the only thing that comes to my mind is subscribeWithScope
, but it's arguable. So feel free to keep it as is, just wanted to note.
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 thinking subscribeAtScope()
, but then I kept misremembering it as subscribeWithScope()
, so I figured we can try it out. Shouldn't be a problem to rename it if we desire down the road.
771a7f7
to
dd10472
Compare
💚 Build Succeeded |
In `ui/public/config/config.js` we have subscription logic that delivers values from an observable synchronously and also ensuring that values are received within a digest cycle. I need to do the same in elastic#23217 but want to keep as few checks for `$rootScope.$$phase` as possible, so I broke the subscription logic into a shared util. In order to make the utility a little more helpful it will also trigger fatal errors if an observable errors without having an error handler, or if `observer.next()`, `observer.error()`, or `observer.complete()` throw, which would normally be swallowed by RxJS.
6.5/6.x: dd716f3 |
In
ui/public/config/config.js
we have subscription logic that delivers values from an observable synchronously and also ensuring that values are received within a digest cycle. I need to do the same in #23217 but want to keep as few checks for$rootScope.$$phase
as possible, so I broke the subscription logic into a shared util.In order to make the utility a little more helpful it will also trigger fatal errors if an observable errors without having an error handler, or if
observer.next()
,observer.error()
, orobserver.complete()
throw, which would normally be swallowed by RxJS.