feat(Fastify) Apollo server Fastify integration #626 #1971
Conversation
ghost
added
the
|
@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
changed the title
rkorrelboom
changed the title
rkorrelboom
changed the title
rkorrelboom
force-pushed
the
apollo-server-fastify-integration
branch
2 times, most recently
from
a58902e
to
7b3f4f9
Compare
|
@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? |
|
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 |
rkorrelboom
force-pushed
the
apollo-server-fastify-integration
branch
2 times, most recently
from
076ff4a
to
3708a88
Compare
rkorrelboom
force-pushed
the
apollo-server-fastify-integration
branch
3 times, most recently
from
ce92aed
to
ef3a3a4
Compare
rkorrelboom
force-pushed
the
apollo-server-fastify-integration
branch
from
ef3a3a4
to
6381c47
Compare
rkorrelboom
force-pushed
the
apollo-server-fastify-integration
branch
from
6381c47
to
f770373
Compare
There was a problem hiding this comment.
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/__tests__/ApolloServer.test.ts
Outdated
Show resolved
Hide resolved
packages/apollo-server-fastify/src/__tests__/ApolloServer.test.ts
Outdated
Show resolved
Hide resolved
|
@abernix Awesome! Thanks for reviewing. If there's anything I can do to help let me know 👍 |
There was a problem hiding this comment.
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
|
@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 I'll have a look at it this friday 👍 |
|
|
|
Well, we almost had a release cut here, but it seems that Ping @addityasingh 👋 (owner of 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 If it's at all acceptable to you, would you consider transferring the package on npm (to myself, or our |
|
This is the most successful skeleton meme I have ever posted anywhere 😭😭😭 Thank you! |
|
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! |
|
@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 |
…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.
…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 I have added both you and apollo-bot, to the list of owners for the
Let me know if you want to remove mine from the list |
Re-land: Fastify integration from #1971.
|
I'm very pleased to announce that 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. |
|
Are you plan realse update for fastify v2? |
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.
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.
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.
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.
fixes #626
TODO: