-
Notifications
You must be signed in to change notification settings - Fork 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
fix(audit, auditTime): audit and auditTime emit last value after source completes #5799
fix(audit, auditTime): audit and auditTime emit last value after source completes #5799
Conversation
const e2subs = ' ----^------------------------!'; | ||
const expected = '-----------------------------|'; | ||
const e2subs = ' ----^-------------------------'; | ||
const expected = '------------------------------'; |
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.
Hmmm... I'll need to think about this. I think this makes the most sense. Given that there are items to audit, and we're waiting for a duration that just never arrives... But I want a second opinion: @cartant
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.
Yeah, I think this makes sense, too. Above all, the behaviour needs to be consistent between operators and, AFAICT, delayWhen
doesn't complete if its notifier is NEVER
:
rxjs/spec/operators/delayWhen-spec.ts
Lines 131 to 144 in 0d3094f
it('should not emit if selector never emits', () => { | |
const e1 = hot('--a--b--|'); | |
const expected = '-'; | |
const subs = '^ !'; | |
const selector = cold( '-'); | |
const selectorSubs = [' ^ ', | |
' ^ ']; | |
const result = e1.pipe(delayWhen((x: any) => selector)); | |
expectObservable(result).toBe(expected); | |
expectSubscriptions(e1.subscriptions).toBe(subs); | |
expectSubscriptions(selector.subscriptions).toBe(selectorSubs); | |
}); |
So this LGTM.
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.
Just a few things, and some open questions for other core team members.
@@ -56,6 +56,7 @@ export function audit<T>(durationSelector: (value: T) => SubscribableOrPromise<a | |||
let hasValue = false; | |||
let lastValue: T | null = null; | |||
let durationSubscriber: Subscriber<any> | null = null; | |||
let isComplete = false; | |||
|
|||
const endDuration = () => { |
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.
@cartant, do we want to fix the duration notifier treating completion like a notification here? Or shall we get that at a later time? The whole thing is weird. Like []
"Here's your value, in this array!". My gut tells me it should be a noop
, and I think we all agreed on that.
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.
Yeah, we should fix it. Whether it's fixed here or someplace else, I don't mind. I intend to do another pass through all of the operators that take a notifier to make sure their behaviour is consistent, etc.
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. @benlesh, I added responses to your comments.
Thanks, @alexsherekin! |
Hello! This change broke our code after update. We use it to detect long press gesture and relied on the fact that audit does not emit last value if observable completes during audit window. Is it possible to add the same configuration as |
Description:
Adjust audit operator to emit the last value when source completes
Related issue (if exists):
#5730