nestjs / graphql

GraphQL (TypeScript) module for Nest framework (node.js) 🍷
https://docs.nestjs.com/graphql/quick-start
MIT License
1.46k stars 397 forks source link

✨ FR: Support for Apollo Server 4 #2445

Closed sschneider-ihre-pvs closed 1 year ago

sschneider-ihre-pvs commented 2 years ago

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

kamilmysliwiec commented 2 years ago

Would you like to create a PR for this issue?

ThomasLohmann commented 2 years ago

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, Thomas

bedcoding commented 2 years ago
  1. I saw the phrase that apollo server v3 is shutting down. If Apollo Server v3 is deprecated, will all the libraries and frameworks that use it be unavailable for 'npm install'? https://www.apollographql.com/docs/apollo-server/v3 스크린샷 2022-11-28 오전 9 08 30

  2. By the way, nestJS, which I am currently using, was also using v3 internally. https://github.com/nestjs/nest/blob/master/package.json 스크린샷 2022-11-28 오전 9 15 37

  3. The official document of nestJS also instructs me to npm install the apollo-server-express that exists in the v3 version. What should I do? I'm so afraid I don't know what I can do to prevent a disaster. https://docs.nestjs.com/graphql/quick-start 스크린샷 2022-11-28 오전 9 09 21

  4. Other official documents are still based on the v3 version. Is there a case of updating nestJS and graphQL to the Apollo v4 version? (How do I update to Apollo v4 version?) https://graphql.org/code/#javascript 스크린샷 2022-11-28 오전 9 55 37

The apollo documentation says to use the '@apollo/server package'. However, no matter how much I googled, I couldn't find any examples of using the @apollo/server package in a nestJS + graphQL project. https://www.apollographql.com/docs/apollo-server/migration 스크린샷 2022-11-28 오전 10 05 31

kamilmysliwiec commented 2 years ago

You can safely keep using v3 for now. It won't be removed, it's just deprecated. We'll eventually migrate @nestjs/apollo to use v4 too

sschneider-ihre-pvs commented 2 years ago

Would you like to create a PR for this issue?

I guess I lack the knowledge to be able to do that.

sschneider-ihre-pvs commented 2 years ago

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

Maxwell2022 commented 1 year ago

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:

luas10c commented 1 year ago

I intend to deliver the upgrade to v4

sam-super commented 1 year ago

@luas10c are you thinking about replacing apollo-server-fastify with: https://www.npmjs.com/package/@as-integrations/fastify as part of the upgrade work?

I would also be keen to help with this if you are interested (i don't have much experience of the codebase though).

sschneider-ihre-pvs commented 1 year ago

I intend to deliver the upgrade to v4

that is good news

luas10c commented 1 year ago

@luas10c are you thinking about replacing apollo-server-fastify with: https://www.npmjs.com/package/@as-integrations/fastify as part of the upgrade work?

I would also be keen to help with this if you are interested (i don't have much experience of the codebase though).

@sam-super Well apollo/server v4 doesn't support fastify layer yet, I'm thinking of upgrading to express layer only

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.

luas10c commented 1 year ago

https://github.com/luas10c/graphql/tree/upgrade-apollo-v4

I'm pushing updates to this branch

timonmasberg commented 1 year ago

https://www.npmjs.com/package/@as-integrations/fastify

Very nice! Can I help in any way? When do you intend to open a PR for this?

luas10c commented 1 year ago

image

Update for @apollo/server v4 is partially ready, I hope you can see the branch and fix possible bugs and test errors.

luas10c commented 1 year ago

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.

luas10c commented 1 year ago

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

rohanrajpal commented 1 year ago

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.

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.

luas10c commented 1 year ago

Much of it is implemented needs to fix tests, if anyone can contribute to this update it will all be welcome.

https://github.com/luas10c/graphql/tree/upgrade/apollo-v4

lpessoa commented 1 year ago

@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?

luas10c commented 1 year ago

@lpessoa

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.

luas10c commented 1 year ago

@lpessoa

If you find more inconveniences let us know, if you have a suggestion on how to get to the solution it will be welcome.

luas10c commented 1 year ago

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.

luas10c commented 1 year ago

We can create a discussion of what would be better to have these dependencies managed by packages or not.

lpessoa commented 1 year ago

@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. Does it sound good to you @kamilmysliwiec ?

luas10c commented 1 year ago

It would be nice to complete the tests, and we deliver a Pull Request all together.

lpessoa commented 1 year ago

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 😃

sschneider-ihre-pvs commented 1 year ago

@kamilmysliwiec can you close https://github.com/nestjs/graphql/pull/2525 that seems outdated and was a draft only anyway

paologf commented 1 year ago

@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 yarn link to link to my project. This are the dependencies in package.json:

  "dependencies": {
    "@nestjs/common": "^9.2.1",
    "@nestjs/config": "^2.2.0",
    "@nestjs/core": "^9.2.1",
    "@nestjs/platform-express": "^9.2.1",
    "@types/node": "^18.11.18",
    "class-transformer": "^0.5.1",
    "class-validator": "^0.14.0",
    "graphql": "^16.6.0",
    "reflect-metadata": "^0.1.13",
    "ts-node": "^10.9.1"
  },
  "devDependencies": {
    "@nestjs/cli": "^9.0.0",
    "@nestjs/schematics": "^9.0.0",
    "@nestjs/testing": "^9.0.0"
  },

My main and app.module files are as following:

// main.ts
import { NestFactory } from '@nestjs/core';
import { AppModule } from './app.module';

async function bootstrap() {
  const app = await NestFactory.create(AppModule);
  await app.listen(3000);
}
bootstrap();

// app.module.ts
import { Module } from '@nestjs/common';
import { AppController } from './app.controller';
import { AppService } from './app.service';
import { ConfigModule } from '@nestjs/config';
import { GraphQLModule } from '@nestjs/graphql-workspace/packages/graphql';
import { ApolloDriver, ApolloDriverConfig } from '@nestjs/graphql-workspace/packages/apollo';

@Module({
  imports: [
   GraphQLModule.forRootAsync<ApolloDriverConfig>({
      driver: ApolloDriver,
    imports: [ConfigModule],
    inject: [ConfigService],
        useFactory: async (configService: ConfigService) => ({
      debug: configService.get<boolean>('GQL_DEBUG', false),
      playground: configService.get<boolean>('GQL_PLAYGROUND', false),
      tracing: configService.get<boolean>('GQL_TRACING', false),
      context: ({ req }) => ({ req }),
      typePaths: ['./**/*.gql'],
    })
    }),
    ConfigModule,
  ],
  controllers: [AppController],
  providers: [AppService],
})
export class AppModule {}

, I got this error:

[Nest] 55  - 02/01/2023, 4:22:09 PM   ERROR [ExceptionHandler] Nest can't resolve dependencies of the GraphQLModule (?, GqlModuleOptions, AbstractGraphQLDriver, GraphQLTypesLoader). Please make sure that the argument HttpAdapterHost at index [0] is available in the GraphQLModule context.

Potential solutions:
- Is GraphQLModule a valid NestJS module?
- If HttpAdapterHost is a provider, is it part of the current GraphQLModule?
- If HttpAdapterHost is exported from a separate @Module, is that module imported within GraphQLModule?
  @Module({
    imports: [ /* the Module containing HttpAdapterHost */ ]
  })

If I restore @nestjs/graphql and @nestjs/apollo ( installing apollo-server-core and apollo-server-express ) it goes fine.

I don't know if the issue is in building/importing your package or something else.. do you have any idea? Thanks

luas10c commented 1 year ago

@paologf

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.

lpessoa commented 1 year ago

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

  1. Setup https://verdaccio.org
  2. Configure verdaccio to use local storage for nest/apollo, nest/grapqhql and nest/mercurius (follow the instructions for verdaccios configuration and add the following to packages definition on verdaccio config yaml)

    
    '@nestjs/apollo':
    access: $all
    publish: $all
    unpublish: $all
    
    '@nestjs/graphql':
    access: $all
    publish: $all
    unpublish: $all

'@nestjs/mercurius': access: $all publish: $all unpublish: $all

3. Start verdaccio
4. Run `learn publish` in nest/graphql project root
```sh
yarn lerna publish --registry=\"http://localhost:4873\" --no-verify-access --no-verify-registry --yes --force-publish
  1. update your project to use verdaccio as package manager/proxy by creating a .yarnrc file with
    registry "http://localhost:4873/"
  2. Update your package.json to use the versions you've generated on step 4

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.

abulhuman commented 1 year ago

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. FYI: I enjoyed reading this entire thread seeing the magic of open source firsthand. I am aware of the progress made by @luas10c , @lpessoa and everybody else involved in here. I only want to know how optimistic I should sound in expecting the v4 upgrade when I pitch it to my boss on the next stand-up.

luas10c commented 1 year ago

We look forward to delivering apollo v4 as soon as possible.

andrewmclagan commented 1 year ago

Thanks for all your hard work @luas10c and everyone else involved. Very appreciated!

paologf commented 1 year ago

@paologf

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.

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). 😉

eonae commented 1 year ago

Thank you all of your work. Much appreciate it. And looking forward to use apollo4 with Nest

lpessoa commented 1 year ago

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 ?

timonmasberg commented 1 year ago

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.

lpessoa commented 1 year ago

@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.

timonmasberg commented 1 year ago

@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?

lpessoa commented 1 year ago

@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' },
          },
        ],
      });
kamilmysliwiec commented 1 year ago

@luas10c could you create a PR with the current progress? I can double-check everything and fil the gaps

luas10c commented 1 year ago

@kamilmysliwiec

We're updating in a separate repository so we can send a PR later with everything ready.

https://github.com/luas10c/graphql

timonmasberg commented 1 year ago

@kamilmysliwiec

We're updating in a separate repository so we can send a PR later with everything ready.

https://github.com/luas10c/graphql

Just open a draft PR, it is more convenient and there the maintainers can already start reviewing and comment on it.

kamilmysliwiec commented 1 year ago

Let's track this here https://github.com/nestjs/graphql/pull/2631

coler-j commented 1 year ago

Is there an issue to track updating the docs to support v4?

kamilmysliwiec commented 1 year ago

Docs won't need many changes (it boils down to swapping dependencies that should be installed/imported in samples - compared to the previous version).