It cancels the polls when the latency > interval

[philjones88]

Take for example in your demo app (https://jiayihu.github.io/rx-polling/demo/).

If you set your latency in the chrome developer tools to >= inteval you end up never getting a successful poll?

Surely there should be an option to "queue" polls so that we can account for latency issues?

(I made a custom profile and set it to 3000ms latency to replicate this)

[jiayihu]

Good catch, I believe the solution would be using concatMap instead of switchMap in line https://github.com/jiayihu/rx-polling/blob/master/index.ts#L77 This way the stream will not abort the previous request and will wait for it to complete or error.
Any PR with the fix and proper tests is welcome, otherwise I'll publish the fix asap myself, probably in the week-end

[philjones88]

I’ll submit a PR tomorrow. I’ve been using a parched version locally with a similar solution in the end.

[philjones88]

I've been trying to work on a fix for this.

However, the concatMap or flatMap or even exhaustMap don't work in this scenario. They don't stop the interval from ticking whilst the request$ is being waited for an emission.

I'm not sure of the best solution. I've rewritten it to be like this in a test branch of our repo:

let firstIteration = true;
let allErrorsCount = 0;
let lastRecoverCount = 0;

const options = Object.assign({}, defaultOptions, userOptions);

const pageVisibilityChange$ = fromEvent(document, 'visibilitychange').pipe(
    startWith(null)
);

const poll$ = request$.pipe(
    retryWhen((errors$) => {
        return errors$.pipe(
            scan(({ errorCount, error }, err) => ({ errorCount: errorCount + 1, error: err }), {
                errorCount: 0,
                error: null
            }),
            switchMap(({ errorCount, error }) => {
                allErrorsCount = errorCount;
                const consecutiveErrorsCount = allErrorsCount - lastRecoverCount;

                // If already tempted too many times don't retry
                if (consecutiveErrorsCount > options.attempts) {
                    throw error;
                }

                const delay = this.getStrategyDelay(consecutiveErrorsCount, options);

                return timer(delay, null);
            })
        );
    })
);

const pollRepeater$ = of({}).pipe(
    delayWhen(() => {
        if (firstIteration) {
            return timer(0);
        }

        return timer(options.interval);
    }),
    switchMap(() => poll$),
    tap(() =>  firstIteration = false),
    repeat()
);

return pageVisibilityChange$.pipe(
    switchMap(() => {
        if (this.isPageActive()) {
            return pollRepeater$;
        }

        // Reset first iteration check as we've destroyed the poller
        firstIteration = true;
        return empty();
    }),
    tap(() => {
        // Update the counter after every successful polling
        lastRecoverCount = allErrorsCount;
    })
);

Which has it's own issues when implemented into this library...

[jiayihu]

I've pushed the fix in 1.0.1, let me know if it is solved. I see you've been trying some changes to the lib, so I'll try to explain what has been changed. Your feedback both from consumer experience and technical reviewer would be much appreciated.

You can see the changes here and they are quite minimal. Basically the approach has been changed into

1. Make the first request and wait it to complete/throw
2. Wait interval time
3. Make the request and wait it to complete/throw
4. Repeat steps 2-3 infinitely

The interval observable has been removed and the polling will repeat the next request only if the previous one has been completed/thrown. This normally should have the same result as before, but if the latency is big, there won't be issues hopefully. Tests seem to confirm the implementation.

It's now important that provided request$ completes, otherwise it will never be repeated but that is actually a desired behaviour. Previously it was repeated nevertheless, but it doesn't make sense to repeat a still in-progress Observable (which caused your problem).

[philjones88]

It's an interesting approach with RXjs that I hadn't thought of. The interval with the take(1) is quite genius.

I think you missed updating the demo files though as I went to quickly test the issue and noticed it still happened.

[jiayihu]

I think you missed updating the demo files though as I went to quickly test the issue and noticed it still happened.

Oh right, should be updated now.

Anyway I've noticed that on error, the request is retried twice instead of once then correctly delayed based on strategy. Will add another patch release later, but it's not a big issue and I know the reason