search

apollographql / graphql-subscriptions

:newspaper: A small module that implements GraphQL subscriptions for Node.js
MIT License
1.59k stars 133 forks source link

Fix Issue #81 Infinite loop after last unsubscribe in withFilter function #84

Closed jansenignacio closed 7 years ago

jansenignacio commented 7 years ago

Fixes #81 and related to apollographql/subscriptions-transport-ws#189

NeoPhi commented 7 years ago

It would be good to have add a test that fails prior to adding this fix.

jansenignacio commented 7 years ago

@NeoPhi can you suggest a way to write a test for this? The steps to reproduce are outlined in this comment: https://github.com/apollographql/subscriptions-transport-ws/issues/189#issuecomment-310724774

maktouch commented 7 years ago

@NeoPhi I updated the commit, added the tests first.

jansenignacio commented 7 years ago

@maktouch and I have already signed the CLA but the CI still shows that it's failing :(

stubailo commented 7 years ago

Confirmed - the CLA has been signed, status failed to update on this PR though.

cncolder commented 7 years ago

This pr fix infinite loop bug. But expose another issus in withFilter.

Sometime payload value is undefined in filterFn(payload.value, args, context, info). User need check rootValue in every filter.

Subscription: {
    serverUpdated: {
      resolve: (rootValue, args, ctx, info) => rootValue,
      subscribe: withFilter(
        () => pubsub.asyncIterator(['serverUpdated']),
        // Check rootValue before use it.
        (rootValue, args, ctx, info) => !!rootValue && rootValue.id === ctx.user.id
      )
    }
}

https://github.com/apollographql/graphql-subscriptions/blob/d8583778db6f408375949e8b34fb70601ff8580d/src/with-filter.ts#L15