allow the filter function to be async

[NeoPhi]

@helfer This was the changed I mentioned to allow the filter function to be async.

[DxCx]

you are ignoring err, is this intended?

[NeoPhi]

As the test case is structured an err would be unexpected but easy enough to check.

[DxCx]

i might be missing something, how do you test async here?

[NeoPhi]

The publish below is for Filter2 which as defined above uses setTimeout to delay returning the result.

[DxCx]

yeah i've seen it, but it means the test is actually making sure promises work,
and not that things are running in parallel..
name is abit confusing.. 🗡

[NeoPhi]

I don't see it as that. If you revert the changes to pubsub.ts this test will fail since and async filter is always truthy even if it ultimately returns a false value. If you have a suggestion about how to restructure the test please let me know.

[DxCx]

oh no, i don't mind at all that this is what you are testing.
i would just rename the test abit to something like "can use promise inside filter function"
or something like that.. since usually when testing 'async', the testing is about being run in parallel

[NeoPhi]

Updated the test name.

[Urigo]

I think you can optimize this code and do a better use of Promise chaining - this way you can avoid the inner promises and the empty resolve(). you can also merge the two catch that does the same thing.

[NeoPhi]

I wasn't able to see an easy way to do that since the context needs to be in the closure for two unrelated calls.

[Urigo]

@NeoPhi, I see, but you think that something like this won't do the trick?

const onMessage = (rootValue) => {
    const contextResult = typeof options.context === 'function' ? options.context() : context;

    return Promise
        .resolve(contextResult)
        .then(context => { filterResult: filter(rootValue, context), context: context} )
        .then(prev=> {
            const {filterResult, context} = prev;

            if (!filterResult) return;

            return execute(
                this.schema,
                parsedQuery,
                rootValue,
                context,
                options.variables,
                options.operationName
            );
        })
        .then(executionResult => {
            options.callback(null, executionResult);
        })
        .catch((error) => {
            options.callback(error);
        });
}

[NeoPhi]

If filter returns a Promise it will not be resolved.

[Urigo]

@NeoPhi , so we can resolve it any way, and do the same for context, and then use Promise.all... something like that:

const onMessage = (rootValue) => {
    const contextResult = typeof options.context === 'function' ? options.context() : context;

    return Promise
        .resolve(contextResult)
        .then(context => Promise.all([ Promise.resolve(filter(rootValue, context)), Promise.resolve(context) ]) )
        .then(promisesResults=> {
            const filterResult = promisesResults[0];
            const context = promisesResults[1];

            if (!filterResult) return;

            return execute(
                this.schema,
                parsedQuery,
                rootValue,
                context,
                options.variables,
                options.operationName
            );
        })
        .then(executionResult => {
            options.callback(null, executionResult);
        })
        .catch((error) => {
            options.callback(error);
        });
}

[NeoPhi]

Woke up with the same thought. Although you can't unnest the executionResult otherwise the callback will be called with (null, undefined) unless that is desired.

[Urigo]

thank you @NeoPhi !