Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

fix(fastify-template): fix for subscriptions in fastify template #2248

Closed
wants to merge 4 commits into from
Closed

fix(fastify-template): fix for subscriptions in fastify template #2248

wants to merge 4 commits into from

Conversation

RinkiyaKeDad
Copy link
Contributor

Fix for #2174

Types of changes

What types of changes does your code introduce to Graphback?

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)
  • Other (please specify)

Checklist

  • I have read the CONTRIBUTING document.
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

As discussed I've added resolvers in the plugin options of mercurius which should fix the issue.

Verification

I tried verifying this by enabling the fastify template and then using the updated cli (by running node dist/index.js in the create-graphback folder) but I received this error due to which the template didn't load and I wasn't able to test it. Please let me know if there are some other steps I can take to test if the subscriptions are working or not. Thanks.
sendNew

Copy link
Contributor

@machi1990 machi1990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I've not verified it as I am away from laptop but it LGTM barring the small requested change inline.

I'll let @craicoverflow @wtrocki do the verification.

@@ -45,6 +45,7 @@ async function start() {

app.register((mercurius as any), {
schema,
resolvers,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think resolvers should include noteResolvers as in

@wtrocki
Copy link
Contributor

wtrocki commented Jan 1, 2021

Let's start this year with this small PR. Looking at it now

@wtrocki
Copy link
Contributor

wtrocki commented Jan 1, 2021

 ! [remote rejected]       pr/RinkiyaKeDad/2248-1 -> pr/RinkiyaKeDad/2248-1 (permission denied)
error: failed to push some refs to 'git@github.com:RinkiyaKeDad/graphback'

I cannot fix issues in PR. Can you add me to fork :) THX

@wtrocki
Copy link
Contributor

wtrocki commented Jan 1, 2021

I tried verifying this by enabling the fastify template and then using the updated cli (by running

How about running this from the code:

cd templates/ts-fastify-mongodb-backend
docker-compose up -d
yarn develop

PS: This doesn't seem to be working for me.

@RinkiyaKeDad
Copy link
Contributor Author

I think resolvers should include noteResolvers as in

Thanks for pointing that out 😅 I've added them now @machi1990.

Can you add me to fork :) THX

Added you @wtrocki :)

PS: This doesn't seem to be working for me.

Same. It doesn't give an error in the terminal like last time, but shows that localhost refused to connect on the browser.

NewTest

@RinkiyaKeDad
Copy link
Contributor Author

RinkiyaKeDad commented Jan 5, 2021

How about running this from the code:

Okay so this way of testing the template works if I don't specify the noteResolvers and simply leave the code like this:

  app.register((mercurius as any), {
    schema,
    resolvers,
    graphiql: true,
    context: contextCreator,
    subscription: true
  });

EDIT : found the reason why it was not working earlier. I had to merge the resolvers rather than simply specifying them in an array. I am now able to run the template @wtrocki. Though the subscriptions are still not working.

But even with this when I tested the subscriptions they were not working. Could it be because makeExecutableSchema is not creating the schema in a way acceptable by mercurius?

Will have to look for some other way to fix this then I guess 🤔

@RinkiyaKeDad
Copy link
Contributor Author

Would it be a good idea to switch from mercurius to apollo-server-fastify?

@wtrocki
Copy link
Contributor

wtrocki commented Jan 9, 2021

Sounds good

@RinkiyaKeDad
Copy link
Contributor Author

Closing this since we're shifting to apollo-server-fastify. See #2256

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants