From 868c02b47bb6f4ec4cd1d68b5b474731c470f27e Mon Sep 17 00:00:00 2001 From: Ben Lesh Date: Mon, 28 Sep 2020 21:40:03 -0500 Subject: [PATCH] fix(delay): Now properly handles Date and negative numbers (#5719) - Resolves an issue where passing a Date would cause every value to be scheduled incorrectly, instead of delaying all values until the absolute date -- as documented. - Resolves an issue where negative numbers were treated as though they were positive numbers - Reduces operator sizes - Adds comments and formatting - Updates tests to reflect better behavior - Adds test for Dates in the past. fixes #5232 --- spec/operators/delay-spec.ts | 69 +++++++++++++++++++-------- src/internal/operators/delay.ts | 83 ++------------------------------- 2 files changed, 53 insertions(+), 99 deletions(-) diff --git a/spec/operators/delay-spec.ts b/spec/operators/delay-spec.ts index a921b3289c..83e5dabc4a 100644 --- a/spec/operators/delay-spec.ts +++ b/spec/operators/delay-spec.ts @@ -6,7 +6,7 @@ import { expect } from 'chai'; import { observableMatcher } from '../helpers/observableMatcher'; /** @test {delay} */ -describe('delay operator', () => { +describe('delay', () => { let testScheduler: TestScheduler; beforeEach(() => { @@ -27,12 +27,26 @@ describe('delay operator', () => { }); }); + it('should not delay at all if the delay number is negative', () => { + testScheduler.run(({ hot, expectObservable, expectSubscriptions }) => { + const e1 = hot('---a--b--|'); + const t = -1; + const expected = '---a--b--|'; + const subs = '^--------!'; + + const result = e1.pipe(delay(t, testScheduler)); + + expectObservable(result).toBe(expected); + expectSubscriptions(e1.subscriptions).toBe(subs); + }); + }); + it('should delay by absolute time period', () => { testScheduler.run(({ hot, time, expectObservable, expectSubscriptions }) => { - const e1 = hot(' --a--b-------------c----d--| '); - const t = time(' -------|'); - const expected = '-------(ab)--------c----d--|'; - const subs = ' ^--------------------------! '; + const e1 = hot(' --a--a---a----a----a------------b---b---b---b--|'); + const t = time(' --------------------|'); + const expected = '--------------------(aaaaa)-----b---b---b---b--|'; + const subs = ' ^----------------------------------------------!'; const absoluteDelay = new Date(testScheduler.now() + t); const result = e1.pipe(delay(absoluteDelay, testScheduler)); @@ -42,12 +56,27 @@ describe('delay operator', () => { }); }); - it('should delay by absolute time period after complete', () => { + it('should not delay at all if the absolute time is in the past', () => { + testScheduler.run(({ hot, expectObservable, expectSubscriptions }) => { + const e1 = hot(' --a--a---a----a----a------------b---b---b---b--|'); + const t = -10000; + const expected = '--a--a---a----a----a------------b---b---b---b--|'; + const subs = ' ^----------------------------------------------!'; + + const absoluteDelay = new Date(testScheduler.now() + t); + const result = e1.pipe(delay(absoluteDelay, testScheduler)); + + expectObservable(result).toBe(expected); + expectSubscriptions(e1.subscriptions).toBe(subs); + }); + }); + + it('should delay by absolute time period after source ends', () => { testScheduler.run(({ hot, time, expectObservable, expectSubscriptions }) => { - const e1 = hot(' ---^--a--b--| '); - const t = time(' ------------|') - const expected = ' ------------(ab|)'; - const subs = ' ^--------! '; + const e1 = hot(' ---^--a-----a---a-----a---|'); + const t = time(' ------------------------------|'); + const expected = ' ------------------------------(aaaa|)'; + const subs = ' ^----------------------! '; const absoluteDelay = new Date(testScheduler.now() + t); const result = e1.pipe(delay(absoluteDelay, testScheduler)); @@ -71,12 +100,12 @@ describe('delay operator', () => { }); }); - it('should raise error when source raises error', () => { + it('should raise error when source raises error before absolute delay fires', () => { testScheduler.run(({ hot, time, expectObservable, expectSubscriptions }) => { - const e1 = hot(' --a--b--#'); - const t = time(' -----------|'); - const expected = '--------#'; - const subs = ' ^-------!'; + const e1 = hot(' --a--a---a-----#'); + const t = time(' --------------------|') + const expected = '---------------#'; + const subs = ' ^--------------!'; const absoluteDelay = new Date(testScheduler.now() + t); const result = e1.pipe(delay(absoluteDelay, testScheduler)); @@ -86,12 +115,12 @@ describe('delay operator', () => { }); }); - it('should raise error when source raises error after subscription when Date is passed', () => { + it('should raise error when source raises error after absolute delay fires', () => { testScheduler.run(({ hot, time, expectObservable, expectSubscriptions }) => { - const e1 = hot(' ---^---a---b-------c----#'); - const t = time(' ---------|') - const expected = ' ---------(ab)---c----#'; - const e1Sub = ' ^--------------------!'; + const e1 = hot(' ---^---a--a---a---a--------b---b---b--#'); + const t = time(' -----------------|'); + const expected = ' -----------------(aaaa)-b---b---b--#'; + const e1Sub = ' ^----------------------------------!'; const absoluteDelay = new Date(testScheduler.now() + t); const result = e1.pipe(delay(absoluteDelay, testScheduler)); diff --git a/src/internal/operators/delay.ts b/src/internal/operators/delay.ts index 3134134d7b..e5040fdcc3 100644 --- a/src/internal/operators/delay.ts +++ b/src/internal/operators/delay.ts @@ -1,9 +1,8 @@ /** @prettier */ import { asyncScheduler } from '../scheduler/async'; -import { isValidDate } from '../util/isDate'; import { MonoTypeOperatorFunction, SchedulerLike } from '../types'; -import { operate } from '../util/lift'; -import { OperatorSubscriber } from './OperatorSubscriber'; +import { delayWhen } from './delayWhen'; +import { timer } from '../observable/timer'; /** * Delays the emission of items from the source Observable by a given timeout or @@ -54,80 +53,6 @@ import { OperatorSubscriber } from './OperatorSubscriber'; * Observable by the specified timeout or Date. */ export function delay(delay: number | Date, scheduler: SchedulerLike = asyncScheduler): MonoTypeOperatorFunction { - // TODO: Properly handle negative delays and dates in the past. - return operate((source, subscriber) => { - const isAbsoluteDelay = isValidDate(delay); - // If the source is complete - let isComplete = false; - // The number of active delays in progress. - let active = 0; - // For absolute time delay, we collect the values in this array and emit - // them when the delay fires. - let absoluteTimeValues: T[] | null = isAbsoluteDelay ? [] : null; - - /** - * Used to check to see if we should complete the resulting - * subscription after delays finish or when the source completes. - * We don't want to complete when the source completes if we - * have delays in flight. - */ - const checkComplete = () => isComplete && !active && !absoluteTimeValues?.length && subscriber.complete(); - - if (isAbsoluteDelay) { - // A date was passed. We only do one delay, so let's get it - // scheduled right away. - active++; - subscriber.add( - scheduler.schedule(() => { - active--; - if (absoluteTimeValues) { - const values = absoluteTimeValues; - absoluteTimeValues = null; - for (const value of values) { - subscriber.next(value); - } - } - checkComplete(); - }, +delay - scheduler.now()) - ); - } - - // Subscribe to the source - source.subscribe( - new OperatorSubscriber( - subscriber, - (value) => { - if (isAbsoluteDelay) { - // If we're dealing with an absolute time (via Date) delay, then before - // the delay fires, the `absoluteTimeValues` array will be present, and - // we want to add them to that. Otherwise, if it's `null`, that is because - // the delay has already fired. - absoluteTimeValues ? absoluteTimeValues.push(value) : subscriber.next(value); - } else { - active++; - subscriber.add( - scheduler.schedule(() => { - active--; - subscriber.next(value); - checkComplete(); - }, delay as number) - ); - } - }, - // Allow errors to pass through. - undefined, - () => { - isComplete = true; - checkComplete(); - } - ) - ); - - // Additional teardown. The other teardown is set up - // implicitly by subscribing with Subscribers. - return () => { - // Release the buffered values. - absoluteTimeValues = null!; - }; - }); + const duration = timer(delay, scheduler); + return delayWhen(() => duration); }