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) Apollo server Fastify integration #626 #1971

Conversation

rkorrelboom
Copy link
Contributor

@rkorrelboom rkorrelboom commented Nov 15, 2018

fixes #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

@ghost ghost added the ⛲️ feature New addition or enhancement to existing solutions label Nov 15, 2018
@apollo-cla
Copy link

@rkorrelboom: 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/

@rkorrelboom rkorrelboom changed the title Initial commit for apollo server fastify integration Initial commit for apollo server fastify integration #626 [WIP] Nov 15, 2018
@rkorrelboom rkorrelboom changed the title Initial commit for apollo server fastify integration #626 [WIP] feat(Fastify) Apollo server Fastify integration #626 [WIP] Nov 15, 2018
@rkorrelboom rkorrelboom changed the title feat(Fastify) Apollo server Fastify integration #626 [WIP] feat(Fastify) Apollo server Fastify integration #626 Nov 15, 2018
@rkorrelboom rkorrelboom force-pushed the apollo-server-fastify-integration branch 2 times, most recently from a58902e to 7b3f4f9 Compare November 15, 2018 12:05
@rkorrelboom
Copy link
Contributor Author

@addityasingh I didn't see any activity on your pr for some time so I started my own feature. While finishing up I saw you picked it up again. I still wanted to give it a shot since I'd love to see a fastify integration for Apollo server. If you have some time, please have a look at this PR. I tried to incorporate the review comments from @mcollina in this branch and stick with the fastify philosophy as much as possible.

@mcollina Could you please have a look at this pr?

@addityasingh
Copy link
Contributor

I was having trouble with Typescript and maybe instead of creating new changeset you could help me out there. I asked @mcollina for help regarding async with preHandler but I can check your changeset and try to fix in mine. Thoughts?

Copy link

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

I did a pass. It's not exactly clear what's the question though.

packages/apollo-server-fastify/README.md Outdated Show resolved Hide resolved
packages/apollo-server-fastify/src/ApolloServer.ts Outdated Show resolved Hide resolved
packages/apollo-server-fastify/src/ApolloServer.ts Outdated Show resolved Hide resolved
@rkorrelboom rkorrelboom force-pushed the apollo-server-fastify-integration branch 2 times, most recently from 076ff4a to 3708a88 Compare November 17, 2018 10:32
README.md Outdated Show resolved Hide resolved
@rkorrelboom rkorrelboom force-pushed the apollo-server-fastify-integration branch 3 times, most recently from ce92aed to ef3a3a4 Compare November 21, 2018 17:20
@rkorrelboom
Copy link
Contributor Author

rkorrelboom commented Nov 21, 2018

@abernix / @mcollina How can we proceed to make sure this branch will be merged? I'm not sure how to move on from here.

I've copied all the tests from express and made sure they succeed for the fastify integration and the current implementation seems to be pretty feature complete.

Copy link

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM from my side.

@rkorrelboom rkorrelboom force-pushed the apollo-server-fastify-integration branch from ef3a3a4 to 6381c47 Compare November 22, 2018 09:56
@abernix abernix self-assigned this Nov 24, 2018
@rkorrelboom rkorrelboom force-pushed the apollo-server-fastify-integration branch from 6381c47 to f770373 Compare November 28, 2018 08:47
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.

This LGTM. I've left a few markers (for myself) with regards to some tweaks which are necessary to align with the work in #2054. Just to be clear: I'm intending on responding/adjusting this PR based on my own feedback here, so you don't need to do anything.

I'm going to try to land that other PR this week — and I'll grab this at the same time — but this looks great otherwise. Thanks!

packages/apollo-server-fastify/src/ApolloServer.ts Outdated Show resolved Hide resolved
packages/apollo-server-fastify/src/ApolloServer.ts Outdated Show resolved Hide resolved
packages/apollo-server-fastify/package.json Outdated Show resolved Hide resolved
@rkorrelboom
Copy link
Contributor Author

@abernix Awesome! Thanks for reviewing. If there's anything I can do to help let me know 👍

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.

I've merged #2054 into a vNEXT staging branch where I expect this will surface as well (as a semver minor v2.3.0) and changed this PR to target that same vNEXT branch (and merged vNEXT into this PR in preparation.)

That said, the tests are currently failing because of the file upload test — and I think I've put it in place correctly. I'll try to take a look at it soon, but if you have a chance to peak, that'd be super helpful!

TypeError: Cannot read property 'singleUpload' of undefined

packages/apollo-server-fastify/package.json Outdated Show resolved Hide resolved
packages/apollo-server-fastify/src/ApolloServer.ts Outdated Show resolved Hide resolved
packages/apollo-server-fastify/src/ApolloServer.ts Outdated Show resolved Hide resolved
@abernix
Copy link
Member

abernix commented Dec 18, 2018

@rkorrelboom Thoughts on disabling the file upload aspect of this Fastify integration until we can circle back to it? I haven't had a chance to investigate yet, but I really want to get this released!

At this point, a 2.4.0 release is under way (which includes some great performance improvements via caching the AST after parsing and validation; see #2111) and I think this could be a candidate for inclusion in that release.

Let me know your thoughts, and no rush either way. I'll still try to find some time to polish this off.

@abernix abernix changed the base branch from master to release-vNEXT December 18, 2018 17:44
@rkorrelboom
Copy link
Contributor Author

@abernix I'll have a look at it this friday 👍

@iagomelanias
Copy link

giphy

@abernix abernix mentioned this pull request Jan 23, 2019
@abernix
Copy link
Member

abernix commented Jan 23, 2019

I can see from the gleaming expression on @brainkim's face in the above photo that they're excited to try this out. Therefore, I'm merging this into #2215 and cutting another 2.4.0 pre-release. I'll ping here when that's done.


Actual footage of me trying to do this previously during the holidays:

giphy-downsized

@abernix abernix merged commit 069110b into apollographql:release-2.4.0 Jan 23, 2019
@abernix
Copy link
Member

abernix commented Jan 23, 2019

Well, we almost had a release cut here, but it seems that apollo-server-fastify already exists on npm: https://www.npmjs.com/package/apollo-server-fastify!

Ping @addityasingh 👋 (owner of apollo-server-fastify according to npm): First, thank you for taking the time to build and provide the apollo-server-fastify integration for Apollo Server that it looks like you put together a while ago. It appears that the current apollo-server-fastify package was built on top of Apollo Server 1.x. In the above PR, you can see that @rkorrelboom has put together a Fastify integration support built on the Apollo Server 2.x framework, and it looks like the last step is finding a place to publish it. The plan had been apollo-server-fastify to match up with our other server-specific integrations (e.g. apollo-server-express, apollo-server-hapi, apollo-server-koa, etc.)

We can of course switch to using a different name, though its certainly worth checking with you first to see if you'd be willing to allow this new implementation to take its place? Is the current apollo-server-fastify integration something you're still interested in maintaining?

If it's at all acceptable to you, would you consider transferring the package on npm (to myself, or our apollo-bot)? I think a Fastify integration that supports Apollo Server 2.x would be a natural upgrade path for anyone currently still on apollo-server-fastify@1, so I hope we can figure something out! Thanks, in advance - at the very least, for your consideration!

@brainkim
Copy link

This is the most successful skeleton meme I have ever posted anywhere 😭😭😭 Thank you!

@abernix
Copy link
Member

abernix commented Jan 30, 2019

As an update to those waiting, I've e-mailed the package author as I haven't received a response elsewhere yet. I'll update with details when I have them, but I do suspect it is worth (e.g. via avoiding confusion) waiting a bit longer if we can take over the existing package!

@nahtnam
Copy link

nahtnam commented Feb 1, 2019

@abernix It might also be worth contacting NPM? I believe they have steps to handle these kinds of situations: https://docs.npmjs.com/misc/disputes.html

abernix added a commit that referenced this pull request Feb 7, 2019
…1971)"

This TEMPORARILY reverts commit 069110b,
which was the result of the work done in #1971 by @rkorrelboom.

Unfortunately, we need to put this on ice while we wait for movement on a
package naming conflict.  The dialog surrounding this is under way, as
I've explained in the PR:

#1971 (comment)

I'm excited to re-land this in an upcoming version, but there's no reason to
block the 2.4.0 release for it right now.

I will open a new PR with the work from #1971 in due time.
abernix added a commit that referenced this pull request Feb 7, 2019
…1971)"

This TEMPORARILY reverts commit 069110b,
which was the result of the work done in #1971 by @rkorrelboom.

Unfortunately, we need to put this on ice while we wait for movement on a
package naming conflict.  The dialog surrounding this is under way, as
I've explained in the PR:

#1971 (comment)

I'm excited to re-land this in an upcoming version, but there's no reason to
block the 2.4.0 release for it right now.

I will open a new PR with the work from #1971 in due time.
@abernix abernix modified the milestones: Release 2.4.0, Release 2.x Feb 7, 2019
@addityasingh
Copy link
Contributor

@abernix I was on vacation till today. Given that this PR has the implementation for fastify, I don't mind transferring the ownership of the package in npm. I will transfer it to you.

@addityasingh
Copy link
Contributor

@abernix I have added both you and apollo-bot, to the list of owners for the apollo-server-fastify package.

~ npm owner ls apollo-server-fastify
abernix npmjs@jro.cc
addi90 adisinghrajput@gmail.com
apollo-bot npm@apollographql.com

Let me know if you want to remove mine from the list

abernix added a commit that referenced this pull request Feb 14, 2019
@abernix
Copy link
Member

abernix commented Feb 14, 2019

I'm very pleased to announce that apollo-server-fastify has been released! apollo-server-fastify@2.4.2 is the first v2 release, and its version number aligns with the current version of the other Apollo integrations.

A big thanks to @addityasingh for transferring it over and letting it live in in its Apollo Server v2-based glory (and hopefully I didn't get in the way of your vacation) and @rkorrelboom for making this PR happen.

@congnt171
Copy link

congnt171 commented Mar 22, 2019

Are you plan realse update for fastify v2?
Thanks!

@abernix abernix removed this from the Release 2.x milestone Jun 28, 2019
glasser added a commit that referenced this pull request Jan 5, 2022
This package was being used only to stringify two small constant
objects. It has a major version update available and rather than
evaluate the CHANGELOG, we might as well just not use it.

(Honestly I think it would be fine to just use JSON.stringify here; I
don't see any justification in #1971 for why this particular use case
requires a faster stringification than all the other JSON
stringification we do in Apollo Server. But just in case this matters to
anybody, I went with the fastest possible way of converting a constant
object to string.)

This should replace #5985.
glasser added a commit that referenced this pull request Jan 5, 2022
This package was being used only to stringify two small constant
objects. It has a major version update available and rather than
evaluate the CHANGELOG, we might as well just not use it.

(Honestly I think it would be fine to just use JSON.stringify here; I
don't see any justification in #1971 for why this particular use case
requires a faster stringification than all the other JSON
stringification we do in Apollo Server. But just in case this matters to
anybody, I went with the fastest possible way of converting a constant
object to string.)

This should replace #5985.
glasser added a commit that referenced this pull request Jan 5, 2022
This package was being used only to stringify two small constant
objects. It has a major version update available and rather than
evaluate the CHANGELOG, we might as well just not use it.

(Honestly I think it would be fine to just use JSON.stringify here; I
don't see any justification in #1971 for why this particular use case
requires a faster stringification than all the other JSON
stringification we do in Apollo Server. But just in case this matters to
anybody, I went with the fastest possible way of converting a constant
object to string.)

This should replace #5985.
glasser added a commit that referenced this pull request Jan 5, 2022
This package was being used only to stringify two small constant
objects. It has a major version update available and rather than
evaluate the CHANGELOG, we might as well just not use it.

(Honestly I think it would be fine to just use JSON.stringify here; I
don't see any justification in #1971 for why this particular use case
requires a faster stringification than all the other JSON
stringification we do in Apollo Server. But just in case this matters to
anybody, I went with the fastest possible way of converting a constant
object to string.)

This should replace #5985.
@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.

9 participants