Skip to content

Commit

Permalink
fix(delay): Now properly handles Date and negative numbers (#5719)
Browse files Browse the repository at this point in the history 
- 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
  • Loading branch information
@benlesh
benlesh authored Sep 29, 2020
1 parent 70ab99f commit 868c02b
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 99 deletions.
69 changes: 49 additions & 20 deletions spec/operators/delay-spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { expect } from 'chai';
import { observableMatcher } from '../helpers/observableMatcher';

/** @test {delay} */
describe('delay operator', () => {
describe('delay', () => {
let testScheduler: TestScheduler;

beforeEach(() => {
Expand All @@ -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));
Expand All @@ -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));
Expand All @@ -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));
Expand All @@ -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));
Expand Down
83 changes: 4 additions & 79 deletions src/internal/operators/delay.ts
View file
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -54,80 +53,6 @@ import { OperatorSubscriber } from './OperatorSubscriber';
* Observable by the specified timeout or Date.
*/
export function delay<T>(delay: number | Date, scheduler: SchedulerLike = asyncScheduler): MonoTypeOperatorFunction<T> {
// 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);
}

1 comment on commit 868c02b

@anulman
Copy link

@anulman anulman commented on 868c02b Feb 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any way to cherry-pick this commit into the 6.x series? I'm seeing a weird bug with misscheduled events while using delay(jsDateObject) and would love to try this fix out.

Please sign in to comment.