-
Notifications
You must be signed in to change notification settings - Fork 388
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
✨ FR: Support for Apollo Server 4 #2445
Comments
Would you like to create a PR for this issue? |
Hi @sschneider-ihre-pvs @kamilmysliwiec any updates regarding this topic? It would be great to finalize it asap, since there are a couple of good improvements similar what GraphQL Yoga already provides: https://www.apollographql.com/blog/announcement/backend/apollo-server-4-a-lightweight-and-easier-to-use-apollo-server/ Best regards, |
The apollo documentation says to use the '@apollo/server package'. |
You can safely keep using v3 for now. It won't be removed, it's just deprecated. We'll eventually migrate |
I guess I lack the knowledge to be able to do that. |
I just had a look and the migration is not that trivial in my opinion since it drops a lot of stuff that is currently used, like the ApolloErrors, and not every error code made it to the replacement enum. Also the applyMiddleware is dropped so the usual app.use is not used and cors and so on have to be applied manually, I couldn't find the express initialization anywhere so I am not sure where to apply the middleware. The list goes on :D |
Yes, it's a tricky one... I myself don't have the full knowledge of how this is working under the hood, but here are few things to consider:
|
I intend to deliver the upgrade to v4 |
@luas10c are you thinking about replacing I would also be keen to help with this if you are interested (i don't have much experience of the codebase though). |
that is good news |
@sam-super This package is pretty cool, but if you don't have a stable version it can be a problem, it seems to be quite young. |
https://github.com/luas10c/graphql/tree/upgrade-apollo-v4 I'm pushing updates to this branch |
Very nice! Can I help in any way? When do you intend to open a PR for this? |
you can, you can open a pull request that we will accept. |
Added a new feature that comes from @nestjs/graphql named GraphQLException to throw exceptions to the apollo graphql server With this, the server can now identify several success messages 200, redirection messages 300, client errors 400 and server errors 500 |
Thanks for efforts on supporting Apollo V4! I guess @timonmasberg meant when do you intend to make a PR to this repository so that your changes can eventually be merged to main. |
Much of it is implemented needs to fix tests, if anyone can contribute to this update it will all be welcome. |
@luas10c I've had a look at your fork and it looks great! I had some issues running it like so: const app = await NestFactory.create<NestFastifyApplication>(AppModule, new FastifyAdapter());
app.listen(3000); The server would not start and after some time (related to fastify plugin timeout configuration) I would get the following error: AvvioError: Plugin did not start in time: 'middlewareWrapper'. You may have forgotten to call 'done' function or to resolve a Promise After some digging I found out the problem resides on the following code: await app.register(cors, options.cors); by removing it the server starts up nicely and the GraphQL endpoint works like a charm. Maybe we should replace this code to use the @fastify/cors package instead. Can I join you on this one? |
All right, correct that part and mention you as co-author of the commit. thank you very much will contribute a lot to the community. |
If you find more inconveniences let us know, if you have a suggestion on how to get to the solution it will be welcome. |
I used dynamic imports, since it's like peerDependencies. If you are using the expression layer you also install the cors dependencies. now if you chose to use fastify you will have to install @fastify/cors in your project. |
We can create a discussion of what would be better to have these dependencies managed by packages or not. |
@luas10c I've started updating the tests to the new APIs. I'll work on my fork from your repo and then create a PR. |
It would be nice to complete the tests, and we deliver a Pull Request all together. |
Yes! I'll open my PR pointing to your fork 😃 |
@kamilmysliwiec can you close #2525 that seems outdated and was a draft only anyway |
@luas10c thanks for your effort! 🍻 I'm trying the package in my project, firs of all I had to remove some git conflicts markers in package.json ( see pull request ) that gives errors during installation, but I still have an issue that I don't understand.. I install and build the package in a container with node18, then I use
My main and app.module files are as following:
, I got this error:
If I restore I don't know if the issue is in building/importing your package or something else.. do you have any idea? |
As you are using a workspace, you can delete the node_modules folder created in your application and install, who manages the dependencies is a global node_modules. Deleting the node_modules folder that should be created in your project will resolve the issue. |
@paologf I had the same issue and used a nifty approach to solve it (maybe not the proper one but hey... it works :) ). I used https://verdaccio.org to proxy my npm/yarn install calls for my sample project. You can follow these steps to do the same:
'@nestjs/apollo':
access: $all
publish: $all
unpublish: $all
'@nestjs/graphql':
access: $all
publish: $all
unpublish: $all
'@nestjs/mercurius':
access: $all
publish: $all
unpublish: $all
yarn lerna publish --registry=\"http://localhost:4873\" --no-verify-access --no-verify-registry --yes --force-publish
registry "http://localhost:4873/"
You will now be able to run your test app with the local changes for the 3 packages inside the nest/graphql monorepo. Hope it helps. |
I was checking out Apollo v4 for a new app since the v3 is not going to last that long and spending any effort on a project with it would be futile piling up unnecessary technical debt since I would be forced to switch to v4 soon. So, I was wondering how far on the roadmap to Apollo v4 + NestJS GraphQL we are. |
We look forward to delivering apollo v4 as soon as possible. |
Thanks for all your hard work @luas10c and everyone else involved. Very appreciated! |
Thanks, this seems working ( @lpessoa no need of verdaccio, but it's something to keep in mind 😊 ). I'll wait till implementations is ended to check something that currently gives me error ( for example emtpy GqlExecutionContext in custom decorators). 😉 |
Thank you all of your work. Much appreciate it. And looking forward to use apollo4 with Nest |
Hi all.. me and @luas10c are almost finished with the tests and will be creating a PR very shortly. We do have some tiny questions though:
What do you guys think ? |
In https://www.apollographql.com/docs/apollo-server/integrations/building-integrations#handle-errors they state that you do not need any special handling for errors with the express integration. IDK how we should handle fastify. |
@timonmasberg we were able to pinpoint to https://github.com/apollographql/apollo-server/blob/28629243c495b6e6caa0da4eaacc8b10c727f2ac/packages/server/src/errorNormalize.ts#L58 , the error thrown does not have any details inside the extension object. Apollo server then just fills it out with an internal server error and no further details. |
Ah I get it, ye, in the changelogs they stated that you have to fill out the extension details on your own. What details do we exactly need there? Isn't the plain GraphqlError enough? |
It should be enough but we do have some test scenarios in the lib that are expecting additional info i.e .expect(200, {
data: null,
errors: [
{
extensions: {
code: 'BAD_USER_INPUT',
response: {
error: 'Bad Request',
message: [
'description must be longer than or equal to 30 characters',
],
statusCode: 400,
},
},
message: 'Bad Request Exception',
},
],
}); was changed to .expect(200, {
data: null,
errors: [
{
message: 'Bad Request Exception',
locations: [{ line: 2, column: 3 }],
path: ['addRecipe'],
// TODO: validate behaviour from apollo v4
extensions: { code: 'INTERNAL_SERVER_ERROR' },
},
],
}); |
@luas10c could you create a PR with the current progress? I can double-check everything and fil the gaps |
We're updating in a separate repository so we can send a PR later with everything ready. |
Just open a draft PR, it is more convenient and there the maintainers can already start reviewing and comment on it. |
Let's track this here #2631 |
Is there an issue to track updating the docs to support v4? |
Docs won't need many changes (it boils down to swapping dependencies that should be installed/imported in samples - compared to the previous version). |
Is there an existing issue that is already proposing this?
Is your feature request related to a problem? Please describe it
There is a RC for Apollo 4 and since the package structure for the express implementation changed I guess there need to be something done to enable support for apollo 4.
Describe the solution you'd like
Make an Apollo Server version selection available so you can decide which implementation to use.
Teachability, documentation, adoption, migration strategy
No response
What is the motivation / use case for changing the behavior?
New Apollo Server 4
The text was updated successfully, but these errors were encountered: