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

feat(fastify): Integrate apollo-fastify plugin #626 #1013

Merged
merged 35 commits into from
Jul 13, 2018

Conversation

addityasingh
Copy link
Contributor

@addityasingh addityasingh commented Apr 30, 2018

Refer #626

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

closes #626

@apollo-cla
Copy link

@addityasingh: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@ghost ghost added ⛲️ feature New addition or enhancement to existing solutions labels Apr 30, 2018
@addityasingh addityasingh changed the title [WIP]: Integrate apollo-fastify plugin #626 Integrate apollo-fastify plugin #626 Apr 30, 2018
@ghost ghost added the ⛲️ feature New addition or enhancement to existing solutions label Apr 30, 2018
@addityasingh
Copy link
Contributor Author

@martijnwalraven @abernix Can you please have a look at this PR? I don't know why the Node 4 and Node 6 builds are breaking as my changes shouldn't even affect those

@Extarys
Copy link

Extarys commented May 3, 2018

Strange, the error comes from Hapi.. 😕

@addityasingh
Copy link
Contributor Author

@Extarys And I have been merging master to my branch because of periodically other changes being merged to master and the master still seems to be breaking for Node 4 and 6

@Extarys
Copy link

Extarys commented May 3, 2018

:/ Huh... I installed fastify yesterday and I'm very excited about this merge - hope it'll eventually works. 😄

@Extarys
Copy link

Extarys commented May 6, 2018

We got an issue here 😢

https://github.com/coopnd/fastify-apollo/issues/15

I converted my koa to fastify because it seemed a little faster but now I cannot continue de develop my app :( I was certain there was already a package for fastify 😨

Copy link
Member

@abernix abernix 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 working on this. I think the problems you're having with Node.js 4 and Node.js 6 are likely due to the introduction of async / await which aren't natively supported on those versions of Node.js (8 was the first which natively supported them without having them transpiled to generators). Therefore, the tsconfig might be a place to start investigating.

@@ -0,0 +1,18 @@
{
"compilerOptions": {
Copy link
Member

Choose a reason for hiding this comment

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

This tsconfig.json should extends the root tsconfig.json, in a similar fashion as seen here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an existing problem at times with extending the tsconfig with fastify, wherein the version of @types/node starts to conflict. I opened an issue in fastify for this, but it was very flaky in behaviour on my machine. Will try to use the extend again and check

@@ -0,0 +1,34 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't seem necessary to introduce a graphql-server-fastify package. These are only kept around because they did exist at one point, but they are now deprecated. Let's just not include these and have apollo-server-fastify be the sole package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I will remove it

yarn.lock Outdated
@@ -0,0 +1,2915 @@
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
Copy link
Member

Choose a reason for hiding this comment

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

Please use npm's package-lock.json, rather than yarn.lock (for consistency with the rest of this repository).

package.json Outdated
@@ -13,6 +13,7 @@
"postinstall": "lerna bootstrap",
"pretest": "npm run compile",
"test": "npm run testonly --",
"tdd": "npm run compile && mocha --full-trace --timeout 5000 ./test/tests.js",
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep this as a separate concern outside of this PR since it's not directly related.

description: Setting up Apollo Server with Fastify
---

[![npm version](https://badge.fury.io/js/apollo-server-core.svg)](https://badge.fury.io/js/apollo-server-core) [![Build Status](https://circleci.com/gh/apollographql/apollo-cache-control-js.svg?style=svg)](https://circleci.com/gh/apollographql/apollo-cache-control-js) [![Coverage Status](https://coveralls.io/repos/github/apollographql/apollo-server/badge.svg?branch=master)](https://coveralls.io/github/apollographql/apollo-server?branch=master) [![Get on Slack](https://img.shields.io/badge/slack-join-orange.svg)](https://www.apollographql.com/#slack)
Copy link
Member

Choose a reason for hiding this comment

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

Aside from the fact that one of these badges is referring to apollo-cache-control-js (something which seems to be the case on the root README.md too and not something you've specifically introduced), let's not include these badges for now.

Copy link
Contributor Author

@addityasingh addityasingh May 16, 2018

Choose a reason for hiding this comment

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

Makes sense. I removed all the badges except the slack and apollo-server coverage. Once this gets published I can add the badges specific to fastify plugin

@addityasingh
Copy link
Contributor Author

addityasingh commented May 16, 2018

@abernix There was an issue with Hapi due to formatting which fixed the Node 6 issue. But the Node 4 issue seems to be totally unrelated to async/await. The actual error seems to be with yarn on CircleCI and I found this similar issue here:

[2/4] Fetching packages...
error [email protected]: The engine "node" is incompatible with this module. Expected version ">=6".
error An unexpected error occurred: "Found incompatible module".
info If you think this is a bug, please open a bug report with the information provided in "/usr/local/share/.config/yarn/global/yarn-error.log".

I also tried committing the package-lock.json to see of it works. I have node 4 locally and it works for me. SO I am not sure if the Docker image run by CircleCI is any different

Logs from local machine

integration:Restify
apolloServer
graphqlHTTP
✓ can be called with an options function
✓ can be called with an options function that returns a promise
✓ throws an error if options promise is rejected
✓ rejects the request if the method is not POST or GET
✓ throws an error if POST body is missing
✓ throws an error if GET query is missing
✓ can handle a basic GET request
✓ can handle a basic implicit GET request
✓ throws error if trying to use mutation using GET request
✓ throws error if trying to use mutation with fragment using GET request
✓ can handle a GET request with variables
✓ can handle a basic request
✓ can handle a basic request with cacheControl
✓ can handle a basic request with cacheControl and defaultMaxAge
✓ returns PersistedQueryNotSupported to a GET request
✓ returns PersistedQueryNotSupported to a POST request
✓ can handle a request with variables
✓ can handle a request with variables as string
✓ can handle a request with variables as an invalid string
✓ can handle a request with operationName
✓ can handle introspection request
✓ can handle batch requests
✓ can handle batch requests
✓ can handle batch requests in parallel (102ms)
✓ clones batch context
✓ executes batch context if it is a function
✓ can handle a request with a mutation
✓ applies the formatResponse function
✓ passes the context to the resolver
✓ passes the rootValue to the resolver
✓ returns errors
✓ applies formatError if provided
Error in formatError function: [Error: I should be caught]
✓ sends internal server error when formatError fails
✓ sends stack trace to error if debug mode is set
✓ sends stack trace to error log if debug mode is set
✓ applies additional validationRules
renderGraphiQL
✓ presents GraphiQL when accepting HTML
✓ allows options to be a function
✓ handles options function errors
✓ presents options variables
✓ presents query variables over options variables
stored queries
✓ works with formatParams
✓ can reject non-whitelisted queries
server setup
✓ throws error on 404 routes
268 passing (4s)
@ posttest /Users/adisingh/Documents/dev/projects/graphql/apollo-server
npm run lint
@ lint /Users/adisingh/Documents/dev/projects/graphql/apollo-server
prettier-check --ignore-path .gitignore "{docs/{,source/},.,packages/,test}/{.js,.ts,*.md}"
All files using prettier code style.
apollo-server [as-fastify-apollo] % node -v
v4.2.6
apollo-server [as-fastify-apollo] %

@addityasingh
Copy link
Contributor Author

@evans After checking the issue #1088, I think it would be a major change in the features supported and looks great. But IMO, I think we should still be able to support fastify with existing version-1 api. I will parallelly create changes for version-2 branch. This way the apollo-fastify plugin would be there for both the versions and it would be incremental. Thoughts?

@addityasingh addityasingh changed the title Integrate apollo-fastify plugin #626 feat: Integrate apollo-fastify plugin #626 Jul 7, 2018
@ghost ghost added the ⛲️ feature New addition or enhancement to existing solutions label Jul 7, 2018
@addityasingh addityasingh changed the title feat: Integrate apollo-fastify plugin #626 feat(fastify): Integrate apollo-fastify plugin #626 Jul 10, 2018
@ghost ghost added the ⛲️ feature New addition or enhancement to existing solutions label Jul 10, 2018
@addityasingh
Copy link
Contributor Author

@abernix @helfer Guys, this PR has been lying there for more than 2 months. Can you please check if something is still missing? I need to use this in one of my projects and would be better to use the standard implementation

@ghost ghost added the ⛲️ feature New addition or enhancement to existing solutions label Jul 13, 2018
@evans evans merged commit 8d5baec into apollographql:master Jul 13, 2018
@evans
Copy link
Contributor

evans commented Jul 13, 2018

@addityasingh We'll get this released in 1.4, thought it will most likely receive very little attention unless the package is included in apollo-server 2.x

@addityasingh
Copy link
Contributor Author

addityasingh commented Jul 13, 2018

@evans Thanks for merging. I am working on the PR for v2.x . Probably will have something by the end of the weekend. Also a nice way to find this can be by updating repo label with fastify and/or fastify-apollo

evans added a commit that referenced this pull request Jul 13, 2018
* chore(deps): update dependency @adonisjs/bodyparser to v2.0.3

* docs: Fix typo in lambda package README (#999)

* chore(deps): update dependency @types/aws-lambda to v8.10.3

* chore(deps): update dependency @types/node to v9.6.7

* chore(deps): update dependency koa to v2.5.1

* chore(deps): update dependency hapi to v17.4.0 (#1009)

* chore(deps): update dependency sinon to v5 (#1010)

* chore(deps): update dependency @types/node to v9.6.8

* chore(deps): update dependency sinon to v5.0.2

* Temporarily remove version 2.

* Update meteor-theme-hexo to 1.0.9.

* chore(deps): update dependency sinon to v5.0.3

* chore(deps): update dependency @types/node to v9.6.9

* Revert "Temporarily remove version 2."

This reverts commit 6f50769.

* Update package.json's hexo.version to 3.7.1.

* chore(deps): update dependency @types/node to v9.6.11

* chore(deps): update dependency @types/node to v9.6.12

* chore(deps): update dependency hexo-server to v0.3.2

* chore(deps): update dependency sinon to v5.0.5

* chore(deps): update dependency sinon to v5.0.6

* chore(deps): update dependency sinon to v5.0.7

* chore(deps): update dependency @types/node to v9.6.14

* chore(deps): update dependency @types/sinon to v4.3.2

* bump version of subscriptions-transport-ws for graphiql

The newer subscription client has a more forgiving default ka timeout

* chore(deps): update dependency @types/node to v9.6.15

* chore(deps): update dependency @types/sinon to v4.3.3

* Fix typos in setup.md

* fix graphiql docs for hapi

* tests: Stop testing Node.js 4, which has reached the end of its LTS. (#1026)

Node.js 4 will no longer be receiving any additional updates from the
Node.js foundation, as per the expected LTS schedule[0] and their blog post:

https://medium.com/the-node-js-collection/april-2018-release-updates-from-the-node-js-project-71687e1f7742

[0]: https://github.com/nodejs/LTS

* chore(deps): update dependency @types/node to v9.6.18

* chore(deps): update dependency @types/aws-lambda to v8.10.4

* chore(deps): update dependency @types/restify to v5.0.8

* chore(deps): update dependency body-parser to v1.18.3

* chore(deps): update dependency koa-bodyparser to v4.2.1

* chore(deps): update dependency mocha to v5.2.0

* chore(deps): update dependency supertest to v3.1.0

* chore(deps): update dependency hapi to v17.5.0

* docs: add pointer to apollo server 2 documentation (#1083)

* chore(deps): update dependency meteor-theme-hexo to v1.0.10

* chore(deps): update dependency @types/aws-lambda to v8.10.5

* chore(deps): update dependency sinon to v5.0.9

* chore(deps): update dependency sinon to v5.0.10

* chore(deps): update dependency @types/node to v9.6.19

* chore(deps): update dependency hapi to v17.5.1

* chore(deps): update dependency typescript to v2.8.4

* chore(deps): update dependency meteor-theme-hexo to v1.0.13 (#1118)

* chore(deps): update dependency @types/aws-lambda to v8.10.6

* chore(deps): update dependency @types/node to v9.6.20

* koa: remove dist names from readme example (#1122)

Since koa@2 has been published for a long time, and almost all the middlewares migrated to koa@2.
It's no need to add extra dist names for koa and koa middlewares.

* chore(deps): update dependency @types/koa to v2.0.46

* chore(deps): update dependency @types/mocha to v5.2.1

* chore(deps): update dependency @types/restify to v5.0.9

* chore(deps): update dependency @types/express to v4.16.0

* chore(deps): update dependency sinon to v5.1.0

* chore(deps): update dependency @types/node to v9.6.21

* chore(deps): update dependency sinon to v5.1.1

* docs: Add/update documentation README.md.

In order to provide a more universal README.md across all documentation
deployment repositories, and most importantly, to reference the so-called
"documentation for the documentation".

* chore(deps): update dependency meteor-theme-hexo to v1.0.14 (#1199)

This Pull Request updates dependency [meteor-theme-hexo](https://github.com/meteor/meteor-theme-hexo) from `v1.0.13` to `v1.0.14`

**Note**: This PR was created on a configured schedule ("after 10pm every weekday,before 5am every weekday" in timezone `America/Los_Angeles`) and will not receive updates outside those times.


<details>
<summary>Release Notes</summary>

### [`v1.0.14`](https://github.com/meteor/meteor-theme-hexo/blob/master/CHANGELOG.md#v1014)
[Compare Source](meteor/meteor-theme-hexo@v1.0.13...v1.0.14)
* Allow the "Edit on GitHub" button to work on "versioned" documentation.
  [PR #&#8203;80](`https://github.com/meteor/meteor-theme-hexo/pull/80`)

---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

* Update renovate.json

* Update renovate.json

Hopefully these additional hours and the offset won't be problematic for anyone, but they should help me out some.

We'll see!

* Update renovate.json

* chore(deps): pin dependency hexo-versioned-netlify-redirects to v1.0.7 (#1203)

This Pull Request updates dependency `hexo-versioned-netlify-redirects` from `^1.0.7` to `v1.0.7`



---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

* chore(deps): update dependency @types/chai to v4.1.4

* chore(deps): update dependency @types/koa-router to v7.0.29

* chore(deps): update dependency @types/mocha to v5.2.3

* chore(deps): update dependency @types/multer to v1.3.7

* chore(deps): update dependency @types/node to v9.6.22

* chore(deps): update dependency meteor-theme-hexo to v1.0.15

* chore(deps): update dependency @types/koa-router to v7.0.30

* chore(deps): update dependency @types/aws-lambda to v8.10.7

* chore(deps): update dependency hapi to v17.5.2

* Use apollo-server v1 in example relating to v1

* docs: Add sentence punctuation to 2.0 callout.

Follows-up on #1083.

* chore(deps): update dependency meteor-theme-hexo to v1.0.16 (#1262)

This Pull Request updates dependency [meteor-theme-hexo](https://github.com/meteor/meteor-theme-hexo) from `v1.0.15` to `v1.0.16`



<details>
<summary>Release Notes</summary>

### [`v1.0.16`](https://github.com/meteor/meteor-theme-hexo/blob/master/CHANGELOG.md#v1016)
[Compare Source](meteor/meteor-theme-hexo@v1.0.15...v1.0.16)
* Re-introduce the scrolling ability within search results.
  [PR #&#8203;83](`https://github.com/meteor/meteor-theme-hexo/pull/83`)

---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

* chore(deps): update dependency @types/mocha to v5.2.4

* chore(deps): update dependency multer to v1.3.1

* chore(deps): update dependency @types/node to v9.6.23

* chore(deps): update dependency apollo-hexo-config to v1.0.8 (#1329)

This Pull Request updates dependency [apollo-hexo-config](https://github.com/apollographql/apollo-hexo-config) from `v1.0.7` to `v1.0.8`



<details>
<summary>Release Notes</summary>

### [`v1.0.8`](https://github.com/apollographql/apollo-hexo-config/compare/v1.0.7...v1.0.8)
[Compare Source](https://github.com/apollographql/apollo-hexo-config/compare/v1.0.7...v1.0.8)


---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

* docs: Add browser auto-reloading on source content changes. (#1336)

By virtue of a relatively simple `hexo-browsersync` package[0], which
implements BrowserSync[1] in Hexo, this change brings support for automatically
reloading the browser when the source content has changed.

No more pressing "Reload" in order to see the changes to the Markdown source
when working on documentation! 🎉

[0]: https://npm.im/hexo-browsersync
[1]: https://www.browsersync.io

* chore(deps): update dependency koa to v2.5.2

* docs: add note about passing context as a function (#757)

* docs: add note about passing context as a function

We realized today (by mistake) that the value of `context` in `GraphQLOptions` can be a function. Adding a note to the docs so it doesn't surprise anyone else.

* docs: update context as a function docs

- fix description per @n1ru4l's feedback
- add a code example of instantiating a new class in the context for each request

* Added option to disable rewriting URL for GraphiQL (#1047)

* added option to disable rewriting url for graphiql

* updated docs

* added link to PR in changelog

* Add skipValidation option. (#839)

* Don't validate if query is already an AST.

* Skip validation option.

* fix(hapi16_support): add hapi 16 next() invocation [closes #744] (#743)

* chore(hapi16_support): add hapi 16 next() invocation

* run lint fix

* update changelog

* feat(fastify): Integrate apollo-fastify plugin (#1013)

* Integrate apollo-fastify plugin #626

* #626 Fix typescript issues

* #626 Update changelog

* #626 Update README

* #626 Fix the breaking tests

* #626 Fix code review comments

* #626 Run Hapi tests only for node 8 and 9

* #626 Run Hapi tests only for node 8 and 9

* #626 Commit package.lock in working state

* #626 Use npm instead of yarn for node 4

* Revert package-lock and circle ci test job steps

* #626 Bump the version

* v1.4.0

* bring version-2 up to date

* remove apollo-server-fastify

* fix export of hapi middleware

* remove setup

* fix hapi readme

* remove unused tests
@brainkim
Copy link

Looks like this PR was removed for releasing purposes? c00918b

@addityasingh
Copy link
Contributor Author

addityasingh commented Aug 7, 2018

@evans I found out that this package name was used by someone else already and requested npmjs support to transfer it to me. I just got a reply from them about the transfer confirmation. Can we re-merge this now, if this was the reason to remove the package in the first place?

screen shot 2018-08-07 at 10 23 17 pm

@addityasingh
Copy link
Contributor Author

I have started now also on v2.x so that we can publish that one asap as well

@abernix
Copy link
Member

abernix commented Aug 16, 2018

@addityasingh 🎊 That's excellent! Thanks for working out the package ownership problem with npm — much appreciated. I suspect publishing problems due to ownership were the cause of the reversal in c00918b (which was reverted in a pinch).

That said, to have this package be a part of this apollo-server monorepo, which I think is a good idea in terms of testing and compatibility, we'll need to get the mdg account added to the apollo-server-fastify package. This ensures that it will be part of the releasing process, but we'd need to limit the access to contributions to that which happens through this repository's normal contribution/review/publish mechanisms.

Just to be clear, this would require that you transfer the package to mdg, and removing your own account from it. You'd still be able to make contributions to it through this repository — though it would require that it go through the normal PR + release process. If we don't do this, we'd risk the published package getting out of date with this repository.

If that sounds okay, please go ahead and open a PR for the 2.0-enabled version and add the mdg user to the apollo-server-fastify package and we can move forward with this! Also happy to discuss further.

const app = fastify();

// jsonParser is needed for POST.
app.addContentTypeParser('application/json', function(req, done) {

Choose a reason for hiding this comment

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

Hi! I'm one of the Fastify maintainers :)
Thank you for implementing this!
Just a nit, you don't need to add a content type parse for application/json, since it's included directly in Fastify!
Besides Fastify performs many security checks that fast-json-body is not doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@delvedor Thanks a lot for the pointer :) But I implemented this 4 months back and for some reason I had to explicitly add the content parser. But I am working on V2 of this plugin and will take your point into consideration while doing that

@Extarys
Copy link

Extarys commented Sep 2, 2018

Can we implement apollo-server-fastify right now? If not, what is the current recommended way while this is finished. Thanks for making this!

@codejie
Copy link

codejie commented Sep 23, 2018

try fastify-apollo-step ..

Can we implement apollo-server-fastify right now? If not, what is the current recommended way while this is finished. Thanks for making this!

maybe try fastify-apollo-step..

@addityasingh
Copy link
Contributor Author

@here Adding here for people who come across this thread: This is the link to the new PR to integrate with V2 WIP PR for apollo-fastify-plugin v2. Currently some tests are failing and I am trying to figure them out. Feel free to add your comments/suggestions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
⛲️ feature New addition or enhancement to existing solutions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support fastify
8 participants