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: add apollo federation support #301

Closed
wants to merge 13 commits into from
Closed

feat: add apollo federation support #301

wants to merge 13 commits into from

Conversation

marcus-sa
Copy link
Contributor

@marcus-sa marcus-sa commented Jun 22, 2019

Note: doesn't support type-graphql because of Apollo's custom directives

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[x] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

#299
#288

What is the new behavior?

Support for Apollo Federation

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@marcus-sa marcus-sa changed the title [WIP] Integration of Apollo Federation Integration of Apollo Federation Jun 22, 2019
@marcus-sa marcus-sa marked this pull request as ready for review June 22, 2019 19:35
@OLDIN
Copy link

OLDIN commented Jun 28, 2019

When will this pull request be accepted?

@marcus-sa
Copy link
Contributor Author

@kamilmysliwiec what's your opinion about this?

@marcus-sa marcus-sa changed the title Integration of Apollo Federation feat: add apollo federation support Jul 9, 2019
@brianschardt
Copy link

When is this going to be added. Super important feature.

@marcus-sa
Copy link
Contributor Author

marcus-sa commented Jul 10, 2019

Seems absurd that PRs are open for weeks without any response from core contributors.

@kamilmysliwiec
Copy link
Member

Seems absurd that PRs are open for weeks without any response from core contributors.

Thanks @marcus-sa for this very valuable input 😄

This feature looks very promising. I'll look at it shortly

@kamilmysliwiec
Copy link
Member

Great idea! Could you add an integration test for this? Also, would you like to create a PR to the documentation with a sample usage? :) Adding an example to nestjs/nest repo (sample dir would be nice too)

@marcus-sa
Copy link
Contributor Author

Sure, I'll do that tomorrow 😁

@brianschardt
Copy link

Any updates? should i keep pushing again super necessary for greenfield projects.

@marcus-sa
Copy link
Contributor Author

marcus-sa commented Jul 17, 2019

@brianschardt there's been some API changes to Apollo Federation that I have to look at first, and then update the PR accordingly :)

https://blog.apollographql.com/announcing-managed-federation-265c9f0bc88e

@sukei
Copy link

sukei commented Aug 22, 2019

Any news about this ?

@marcus-sa
Copy link
Contributor Author

marcus-sa commented Sep 2, 2019

@sukei been busy at work and with other projects, but essentially I'm waiting for TypeGraphQL to support custom directives which is required by Apollo Federation.
If this PR is really needed as it is, I can make an exception and finish it up without code-first support.

@eole1712
Copy link

eole1712 commented Sep 2, 2019

@marcus-sa it would really be great to be able to merge this without waiting for the code-first support

} = this.options;
const app = httpAdapter.getInstance();

const typeDefs = await this.graphqlTypesLoader.getTypesFromPaths(typePaths);
Copy link

Choose a reason for hiding this comment

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

@marcus-sa just a little question, why are u here getting types instead of merging them with mergeTypesByPaths like in graphql.module.ts ?

@greben99
Copy link

greben99 commented Oct 22, 2019

Hey Guys, just wondering how this is looking? Is there anything still blocking this. I agree with @eole1712 above, when TypeGraphQL comes out with support, can look at another pull request to implement that.

@renepardon
Copy link

renepardon commented Oct 24, 2019

I want to switch from PHP (laravel) to NestJS but this is a show stopper. What's the current state?

@greben99
Copy link

I want to switch from PHP (laravel) to NestJS but this is a show stopper. What's the current state?

I just forked it and merged this pull request into my fork and using the fork in the interim, as hopefully before we go live, it will be merged with master.

@renepardon
Copy link

I want to switch from PHP (laravel) to NestJS but this is a show stopper. What's the current state?

I just forked it and merged this pull request into my fork and using the fork in the interim, as hopefully before we go live, it will be merged with master.

I forked the fork from @marcus-sa and tried to get it running with "@nestjs/graphql": "file:lib/graphql", within my package.json file but receive a lot of errors like:

This module is declared with using 'export =', and can only be used with a default import when using the 'esModuleInterop' flag

lib/graphql/node_modules/apollo-server-express/node_modules/apollo-server-core/src/types/graphql-upload.d.ts:4:16 - error TS2451: Cannot redeclare block-scoped variable 'GraphQLUpload'.

lib/graphql/node_modules/apollo-server-express/node_modules/apollo-server-core/src/types/graphql-upload.d.ts:21:15 - error TS2300: Duplicate identifier 'Request'.

lib/graphql/node_modules/apollo-server-express/node_modules/apollo-server-core/src/types/graphql-upload.d.ts:23:15 - error TS2300: Duplicate identifier 'Response'.

lib/graphql/node_modules/apollo-server-core/src/types/graphql-upload.d.ts:23:15 23 export type Response = any; ~~~~~~~~ 'Response' was also declared here.

How to reproduce:

First I adjusted the package.json file to contain this line:

"@nestjs/graphql": "file:lib/graphql",

Then executed those commands:

mkdir -p lib/graphql
git clone https://github.com/marcus-sa/graphql.git lib/graphql
cd lib/graphql
npm i && npm audit fix
npm run build
cd ../..
rm -rf node_modules package-lock.json
npm i
npm run start:debug

I also tried to add the lib folder within my project tsconfig.json to exclude but makes no difference.

@tuxmachine tuxmachine mentioned this pull request Oct 29, 2019
3 tasks
@marcus-sa
Copy link
Contributor Author

marcus-sa commented Oct 29, 2019

Thanks to @tuxmachine for following up on it.
You can view the finalized PR here
#455

@lock
Copy link

lock bot commented Apr 25, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants