fix: ensure catchError functions always return source iterator

[jeengbe]

catchError currently swallows return signals. Instead of explicitly returning only if the source iterator is done or an error is thrown, always return it once done.

I have briefly tried to add unit tests for this, but I couldn't find any other tests for similar behaviour and couldn't manage the juggle to spy on the return function.

[jeengbe]

☹️

yarn install v1.22.22
[1/4] Resolving packages...
[2/4] Fetching packages...
yarn run v1.22.22
$ eslint src spec
/bin/sh: 1: eslint: not found
error Command failed with exit code [12](https://github.com/ReactiveX/IxJS/actions/runs/10006204785/job/27677171680#step:7:13)7.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Error: Process completed with exit code 127.

[trxcllnt]

That's strange. Looking into it.

[trxcllnt]

I don't know what's going on. It doesn't look like yarn is fetching eslint at all. It's possible something in the request chain is affected by the numerous outages today.

[trxcllnt]

Tests passed, LGTM. @mattpodwysocki any thoughts?

[jeengbe]

Could you please take care of the change log and publish an update once this is merged? Thanks 🙂

[trxcllnt]

Yes, changelog happens automatically when we publish. @mattpodwysocki requests a test to exercise the new behavior.

The tests are in the spec dir, i.e. spec/iterable-operators/catcherror-spec.ts.

I think something like this should work for Iterable and can adapt for AsyncIterable:

test('Iterable#catchError calls return() on source iterator when no error', () => {
  let done = false;
  let returnCalled = false;
  const xs = {
    [Symbol.iterator]() { return xs; },
    next: () => (done = !done) ? { value: 0 } : { done: true },
    return: () => { returnCalled = true; }
  };
  const res = from(xs).pipe(catchError((_: Error) => of('foo')));
  expect(sequenceEqual(res, range(0, 1))).toBeTruthy();
  expect(returnCalled).toBeTruthy();
});

You can run the tests against the source via:

yarn test -t src --tests spec/iterable-operators/catcherror-spec.ts

Or to test against the compiled code:

# Compile all the output targets
yarn build
# Run the given test against each target
yarn test --tests spec/iterable-operators/catcherror-spec.ts

[jeengbe]

How do I debug tests? The tests fail for some reason, and I can't figure out how to attach a debugger, or even use console.log statements.
I tried console.log(CatchWithIterable.toString()) to check whether it's even executing the right code, but not even that log shows up in the console.

Also, if unrelated, I can't use jest in tests or I get ReferenceError: jest is not defined.

[trxcllnt]

You can debug the tests in VSCode via the "Debug Unit Tests" launch target.

1. Install the augustocdias.tasks-shell-input VSCode extension
2. In the "debugger" view, select "Debug Unit Tests" from list of launch targets:
   
3. Hit the green "play" icon to start debugging
4. Select "src" for the target to debug from the list:
   
5. Type in/select the test file to debug from the list:
   

[trxcllnt]

I don't think we typically rely on the jasmine spy functions, but I do see one place we import and use them in aborting-spec.ts.

[trxcllnt]

We pass --reporters=jest-silent-reporter to jest by default, unless you run the tests with --verbose:

yarn test -t src --tests spec/iterable-operators/catcherror-spec.ts --verbose

I suspect this is why you weren't seeing your console.log() statements.

[trxcllnt]

This PR has me wondering if the style of explicitly constructing and enumerating the iterator should be avoided.

Perhaps we should refactor to be something like this?

let count = this._count;
for (const value of this._source) {
  if (--count < 0) {
    yield value;
  }
}

[jeengbe]

I fully agree with that. Virtually every manual call to [Symbol.(async)Iterator]() is susceptible to this issue.

That's for another MR, though. I need this change to go through first (it's left a big "TODO: update ix" at work), but could help with fixing the remaining ones after this.

[trxcllnt]

I'm debugging the tests and it looks like we've stumbled across a closure compiler bug (wouldn't be the first time). I'll test if changing the implementation to not iterate manually fixes the issue.

[trxcllnt]

Oh actually, I know the issue. Closure compiler's iterator rewriting replaces calls like it.return() with returnIterator(it), meaning the jest.spyOn(xs, 'return') never gets called (even though the iterators return() logic does get executed). So we'll have to change the test to check a flag like in my example above.

[trxcllnt]

Ah, I spoke too soon. It is indeed a closure compiler bug:

// test.js
function first(xs) {
    for (let x of xs) {
        return x;
    }
}

let count = 0;
let returnCalled = false;

const xs = {
    [Symbol.iterator]() { return xs; },
    next() {
        if (count < 3) {
            return { value: count++, done: false };
        }
        return { done: true };
    },
    return() {
        returnCalled = true;
        return { done: true };
    }
};

first(xs);

console.log('return called:', returnCalled);

$ npx google-closure-compiler --js test.js --js_output_file test.min.js --language_in ECMASCRIPT_NEXT --language_out ECMASCRIPT5 --assume_function_wrapper --compilation_level ADVANCED --third_party true
$ node test.js
> return called: true
$ node test.min.js
> return called: false

[jeengbe]

How do we proceed here? We could
a) quickfix the test by using a local implementation of first that cc doesn't break
b) modify first() to account for the same issue and go directly against #373 (comment)
c) replace cc with tsc --downlevelIteration? What's the reason cc is used in the first place, to reduce bundle size?

[trxcllnt]

Your options A and B won't work, because the issue isn't with our code -- even if we implement a manual iterator, the issue is that closure compiler doesn't call the return() function when it breaks or exits a for...of loop. The only solution is option C, stop using closure-compiler.

The reason for closure-compiler is that historically, its (async)iterator and Promise polyfills have been both faster and smaller than alternatives, so we've tolerated its difficulties. That said, I haven't compared it to the downleveled iterators that use @swc/helpers instead of tslib, so maybe that's worth investigating again.

[jeengbe]

Why does Ix provide es5 code at all in the first place? for await...of came with Node 10, which reached eol years ago. I assume browser bundlers do their own pass of converting to es5, so shipping es5 code is penalising performance for most users at the benefit of nothing?