Skip to content
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

Zeit micro support #347

Merged
merged 13 commits into from
Apr 8, 2017
Merged

Zeit micro support #347

merged 13 commits into from
Apr 8, 2017

Conversation

nnance
Copy link
Contributor

@nnance nnance commented Apr 2, 2017

This adds support for Zeit micro to allow GraphQL to be served as a function. Fixes issue #324

TODO:

  • Update CHANGELOG.md with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

@nnance nnance requested review from DxCx, stubailo and helfer April 2, 2017 14:38
@soda0289
Copy link
Contributor

soda0289 commented Apr 2, 2017

I think we might need an example on the README and update the list of integrations.

@nnance
Copy link
Contributor Author

nnance commented Apr 2, 2017 via email

Copy link
Contributor

@DxCx DxCx left a comment

Choose a reason for hiding this comment

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

hey! looks great in general.. few questions here and then ;)

let query;
if (req.method === 'POST') {
try {
query = await json(req);
Copy link
Contributor

Choose a reason for hiding this comment

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

await on json? oh?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

json is a function supplied with micro that is an async parser. the reason it is async is because it will use the request as a stream and will return the complete json once the full request payload is received.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh cool :)

try {
query = await json(req);
} catch (err) {
query = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

and then? i mean, where do you expect the server to respond with an error?

Copy link
Contributor Author

@nnance nnance Apr 3, 2017

Choose a reason for hiding this comment

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

actually by setting query to undefined it causes the runHttpQuery function to trigger an error. This is fundamentally what happens with the express middleware when the body is empty or not parsable

@@ -9,6 +9,7 @@ require('../packages/graphql-server-express/dist/connectApollo.test');
require('../packages/graphql-server-hapi/dist/hapiApollo.test');
if (NODE_MAJOR_VERSION >= 6) {
require('../packages/graphql-server-koa/dist/koaApollo.test');
require('../packages/graphql-server-micro/dist/microApollo.test');
Copy link
Contributor

Choose a reason for hiding this comment

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

micro does not support es5?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no. it uses async await.

},
"devDependencies": {
"graphql-server-integration-testsuite": "^0.6.0",
"micro": "^7.3.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

does micro package has embeded typings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no

"micro": "^7.3.0"
},
"peerDependencies": {
"graphql": "^0.8.0 || ^0.9.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

i would also add micro here..

"graphql": "^0.8.0 || ^0.9.0"
},
"optionalDependencies": {
"@types/graphql": "^0.9.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

and @types/micro if such needed (see question above :))

.gitignore Outdated
@@ -27,7 +27,6 @@ build/Release

# Dependency directory
node_modules
typings
Copy link
Contributor

Choose a reason for hiding this comment

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

why? :O

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isn't this old / left over from the old typings from the old version of typescript?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, but sometimes people still downloading typings locally, just wanted to make sure you didnt remove this to commit typings into the repo

Copy link
Contributor

@helfer helfer left a comment

Choose a reason for hiding this comment

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

This looks great @nnance , you can merge it as is! 🙂

@@ -7,6 +7,7 @@
* Restify: Fix for calling next() ([@jadkap](https://github.com/jadkap)) on [#285](https://github.com/apollostack/graphql-server/pull/285)
* Update GraphiQL to version 0.9.1 ([@ephemer](https://github.com/ephemer)) on [#293](https://github.com/apollostack/graphql-server/pull/293)
* Add AWS Lambda Integration [#101](https://github.com/apollostack/graphql-server/issues/101)
* Add Zeit Micro Integration [#324](https://github.com/apollographql/graphql-server/issues/324)
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we really not released since adding AWS lamdba or did we (I?) just forget to update the changelog with the latest version?

throw error;
}

if ( error.headers ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this something we added recently? I don't remember having this in the initial versions. What are people using those headers for?

@nnance nnance merged commit 45aa44f into master Apr 8, 2017
@nnance nnance deleted the zeit-micro branch April 8, 2017 21:18
import {IncomingMessage, ServerResponse} from 'http';

export interface MicroGraphQLOptionsFunction {
(req?: IncomingMessage): GraphQLOptions | Promise<GraphQLOptions>;
Copy link
Contributor

Choose a reason for hiding this comment

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

why the req parameter is considered optional?

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants