Skip to content

Parse callbacks from Open API and create subscriptions #311

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 19 commits into from
Apr 23, 2020

Conversation

Alan-Cha
Copy link
Collaborator

@Alan-Cha Alan-Cha commented Apr 6, 2020

Related to #297

@Alan-Cha
Copy link
Collaborator Author

Alan-Cha commented Apr 6, 2020

I had to rename the new API to Example API 7 as there already exists and 5 and a 6.

Unfortunately, I am still having some issues with the test. I get the following error from Jest when I run the test in isolation:

Jest did not exit one second after the test run has completed.

This usually means that there are asynchronous operations that weren't stopped in your tests. Consider running Jest with `--detectOpenHandles` to troubleshoot this issue.

Try running npm test api7 and let me know if you get the same error. I'm not exactly sure how to resolve this issue. It also seems to be a pain to use the suggested option as I'm not sure how to configure the Jest CLI to work with TypeScript.

@getlarge
Copy link
Contributor

getlarge commented Apr 7, 2020

@Alan-Cha the code changes looks good ;)
I will give you more feedbacks on friday.

@getlarge
Copy link
Contributor

I merged your changes, and fixed the test for api7. The TCP server was not closed properly, i also changed jest config as it might be useful in the future to track those workers not closed properly.
The new processOperation method is really nice !

getlarge and others added 3 commits April 20, 2020 14:35
@Alan-Cha Alan-Cha force-pushed the callbacks_subscription3 branch from a12cfff to f545336 Compare April 20, 2020 18:35
@Alan-Cha
Copy link
Collaborator Author

Alan-Cha commented Apr 20, 2020

I've created a new README.md file to explain the different example APIs. Hopefully this will at least clear up some of the confusion.

@Alan-Cha Alan-Cha force-pushed the callbacks_subscription3 branch from d887e06 to 3e5beb7 Compare April 21, 2020 17:06
@getlarge
Copy link
Contributor

@Alan-Cha Good we're almost there ;)

For the customResolvers in Subscription, i don't see where the conflicts could happen, could you explain please ?
Since callbacks and those specific resolvers would be used together only if the option createSubscriptionsFromCallbacks is true, the user should be conscious that his callbacks will be dedicated to create Subscription fields ?

@Alan-Cha
Copy link
Collaborator Author

Alan-Cha commented Apr 22, 2020

@getlarge Hey! Some tests were still failing after your changes but I managed to get them working again! I thought it may have to do with inconsistent package versions across the different testing environments so I played around with the configuration and it seemed to do the trick!

My concern with customResolvers is that in the current iteration, you can only define a resolver for either the operation or the callback (that is on the operation) but you cannot do both. I think we should create a new option called customSubscriptionResolvers and move things around to separate Query/Mutation resolvers and Subscription resolvers/subscribes and to make it explicitly clear. Thoughts? I'll take a swing at it.

@getlarge
Copy link
Contributor

@Alan-Cha I will try the tests on your branch so that you have some additional feedback for the 'trick'.

Thanks for the explanation on the customResolvers, i didn't notice this problem.
Anyway it makes sense to have another object customSubscriptionResolvers since they won't have the same properties and both resolvers could even have their own type.

@Alan-Cha
Copy link
Collaborator Author

Alan-Cha commented Apr 22, 2020

@getlarge Don't worry about the tests! In my opinion, we should have tried to keep package versions consistent in our Travis tests from the beginning. There are no more failing tests now so I think we should move on. I also could not replicate the failures that were showing up in Travis previously.

I added the customSubscriptionResolvers option so I think we're pretty much set to pull and publish!

Copy link
Contributor

@getlarge getlarge left a comment

Choose a reason for hiding this comment

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

Seems good to me

@@ -519,7 +517,8 @@ async function translateOpenAPIToGraphQL(
}, 0)

/**
* Organize created queries / mutations / subscriptions into viewer objects.
* Organize authenticated Query, Mutation, and Subscriptions fields into
* viewer objects.
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 that quite soon we will see the issue of the auth for Subscription, because for now i think it's a rather limited because it's based on HTTP security schemes that will only apply to Websocket eventually.
Usually for a subscription, the authentication takes place a connection time, from parameter set in the PubSub client instance. But then Pubsub instance like eventEmitter don't take auth parameter...
So those authViews could set the connection params, if it's possible ... As you can see it might require some thoughts and tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I see! We should file an issue for this. I can get started on it and if you'd like to add anything, feel free to do so! But for this PR, I think we can go ahead :)

Signed-off-by: Alan Cha <[email protected]>
@Alan-Cha Alan-Cha force-pushed the callbacks_subscription3 branch from bf41365 to f2fa8c2 Compare April 23, 2020 14:09
@Alan-Cha
Copy link
Collaborator Author

I made some final changes generally centered around comments. Once the tests run, I will publish!

@Alan-Cha Alan-Cha merged commit c43b783 into IBM:master Apr 23, 2020
@Alan-Cha
Copy link
Collaborator Author

Looks like the tests have finished running! It's finally in! Thank you so so so much again! I know it's taken a long time to get to this point but I'm happy we finally got here! This has been the biggest PR we've ever had so it is really an incredible achievement. It has been a pleasure working with you @getlarge 🥳🍾🎆

@Alan-Cha
Copy link
Collaborator Author

And it's out!!!

Release notes

@getlarge
Copy link
Contributor

Very nice !! I already updated some of my package.json ;)
@Alan-Cha Pleasure was mine too and thanks for your improvements and your time spent on reviews.
See ya on the next issue / PR ;)

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

Successfully merging this pull request may close these issues.

2 participants