withFilter's filterFn is called with a null payload

[green-coder]

withFilter()'s filterFn is called with a null payload when it should not even be called.

I strongly suspect (because I took a look at its source code) that it is called when the async iterator is returning an object that says that there is no more values.

    const getNextPromise = () => {
      return asyncIterator
        .next()
        .then(payload => Promise.all([
          payload,
          Promise.resolve(filterFn(payload.value, args, context, info)).catch(() => false),
        ]))
        .then(([payload, filterResult]) => {
          if (filterResult === true || payload.done === true) {
            return payload;
          }

          // Skip the current value and wait for the next one
          return getNextPromise();
        });
    };

That's wrong, the filter function should not be called when there is no more value in the iterator.

payload.done === true should be tested before calling filterFn.

[green-coder]

I would like to point out that the last object sent by an async iterator is normally {value: undefined, done: true}, it does not convey the last item of the iteration.

[NeoPhi]

This is tricky since the JS specification for iterators allows a value to be included even when done == true.

From iterator protocol Has the value true if the iterator is past the end of the iterated sequence. In this case value optionally specifies the return value of the iterator.

[green-coder]

Not so tricky : currently, the filter is called with an undefined value. This is a bug.

The fix could be adapted to test for an undefined value, but the best is to make sure not to use the ambiguity of the protocol. What if the data's last element is an undefined value, how would you know if there is a data or not?

[green-coder]

Okay .. maybe one could test if the key value is present in the object.

[green-coder]

I will add another commit to take into account the information you gave and handle the special case you mentioned.

[grantwwu]

I suspect that the root cause is completely different. https://github.com/apollographql/graphql-subscriptions/blob/master/src/with-filter.ts#L13 we are getting payload.value before we even await payload. payload.value would end up being undefined, always.

[grantwwu]

I reviewed the code and it LGTM. Rebased and merged.

[grantwwu]

Okay, so I was wrong. payload at this line https://github.com/apollographql/graphql-subscriptions/blob/588438089336bb88368f0f752bdfce59376239f4/src/with-filter.ts#L14 is not a Promise - but that's okay - Promise.all actually just silently ignores non-promise values in the iterable argument it supplies.

Still - I believe the fix is correct - the old version of the code unconditionally calls the filterFn - even when payload.value is undefined.

[done] has the value true if the iterator is past the end of the iterated sequence. In this case value optionally specifies the return value of the iterator.

It's not actually clear to me that it makes sense to apply the filterFn to the return value of the iterator. (what does that even mean, exactly?) I would think that we should only be applying it to elements yielded by the iterator?

@NeoPhi Thanks for catching this - what do you think?

[NeoPhi]

In looking at how the output of this consumed https://github.com/graphql/graphql-js/blob/master/src/subscription/mapAsyncIterator.js#L35 it doesn't look like the calling code considers what happens if a value is set when done is set. Given that the change to check and quick bail when done is set makes sense, even if not 100% accounting for how the JS spec is written.