graphql subscriptions broken

[Warchant]

Bug Report

Input Code

Graphql documentation about subscriptions is not complete.

Documentation shows us example of defining subscriptions, but it does not show us example of firing new event.

const pubSub = new PubSub();

@Resolver('Author')
export class AuthorResolver {
  constructor(
    private readonly authorsService: AuthorsService,
    private readonly postsService: PostsService,
  ) {}

  @Query(returns => Author, { name: 'author' })
  async getAuthor(@Args({ name: 'id', type: () => Int }) id: number) {
    return await this.authorsService.findOneById(id);
  }

  @ResolveProperty('posts')
  async getPosts(@Parent() author) {
    const { id } = author;
    return await this.postsService.findAll({ authorId: id });
  }

  @Subscription(returns => Comment)
  commentAdded() {
    return pubSub.asyncIterator('commentAdded');
  }
}

Current behavior

Regular user will just do

const comment: Comment = {...};
pubSub.publish('commentAdded', comment);

and it will fail with "<some required> field is null" error when Comment has some non nullable fields, because actually response should replicate request structure:

{
  // note subscription name
  commentAdded {
    ... (comment fields here)
  }
}

and by-default when you publish some JS object, it gets into the subscription response as is (e.g. without commentAdded key)

Expected behavior

Users should publish this instead:

const comment: Comment = {...};
pubSub.publish('commentAdded', {
  // this key should be the same as subscription method name in resolver (e.g. commentAdded)
  commentAdded: comment
});

In my opinion this is an issue, because it couples method name in resolver and event (payload) structure.

Possible Solution

One solution could be to use resolve, but this actually does not work and also fails with the same "<some required> field is null" error.

@Subscription(returns => Comment, {
  // this does not work
  resolve: payload => ({
    commentAdded: payload  
  })  
})
commentAdded() {
  return pubSub.asyncIterator('commentAdded');
}

But users should keep in mind that they need to write this "resolve" method every time.

Better solution would be to use reflection to get method name and do this transformation automatically (and actually hide this from end user).

Environment

Nest version: 6.2.4


For Tooling issues:
- Node version: v11.1.0  
- Platform:  Mac

[gimyboya]

Run into the same issue and took me a while to figure it out

[Warchant]

I think it would be much better to use rxjs observables with pubsub, so you don't write filter and resolve in the annotation, but you filter and map observable values. Then, just map it to AsyncIterator<T>

[kamilmysliwiec]

We wrap Apollo GraphQL server inside and these requirements (both usage of AsyncInterator instead of RxJS and the event structure) are coming from this library. I'd suggest creating an issue in their repo.

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.