-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add support for subscriptions in GraphiQL #320
Add support for subscriptions in GraphiQL #320
Conversation
I have it already working its on the other PR... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me. Do you have any specific feedback @Hagai? This PR is much easier to merge because it just does one relatively small thing. Let's talk soon to see how we can split your PR to merge it in parts as well!
@Urigo please ping me once the CI tests pass, then I can review again. |
@@ -66,6 +68,7 @@ export function renderGraphiQL(data: GraphiQLData): string { | |||
<script src="//cdn.jsdelivr.net/react/15.0.0/react.min.js"></script> | |||
<script src="//cdn.jsdelivr.net/react/15.0.0/react-dom.min.js"></script> | |||
<script src="//cdn.jsdelivr.net/graphiql/${GRAPHIQL_VERSION}/graphiql.min.js"></script> | |||
<script src="//unpkg.com/subscriptions-transport-ws@0.5.5/browser/client.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to pull the package when the user didnt provide the websocket endpoint?
|
||
var fetcher; | ||
|
||
if ('${subscriptionsEndpoint}' !== '') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing, textal if over known value..
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "graphql-server-module-graphiql", | |||
"version": "0.4.4", | |||
"version": "0.6.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, do not.
this should be managed by lerna when running npm run release
;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have issues with the build because someone already broke it (the same way this does), #334 fixes just that.
So please revert this version change, and we will rebase and merge this
Create an integration that will return a lambda handler for a graphql or graphiql server. This integration requires an API Gateway with Lambda Proxy Integration. The test runner expects req and res objects for testing. We have to mock these for the tests to execute properly.
Add example lambda handler and some documentation
This adds basic instructions to the README for deploying the lambda function. It uses the Serverless Application Model by creating a template that is used by the AWS CLI.
finally, lerna/lerna#507 fixed the devDep issue.
# Conflicts: # lerna.json # package.json # packages/graphql-server-lambda/README.md # packages/graphql-server-lambda/src/lambdaApollo.ts
@DxCx removed the version change commit, this PR is ready. Thank you and see you tomorrow! |
…l-server into feat/subscriptions-graphiql
Add support for subscriptions in GraphiQL using subscriptions-transport-ws.
Waiting for apollographql/subscriptions-transport-ws#96