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

fix(buildFederatedSchema): Export GraphQLSchemaModule type #293

Merged
merged 3 commits into from
Dec 15, 2020

Conversation

jtomaszewski
Copy link
Contributor

We have a big app that uses @apollo/federation and we have several modules defined separately, with their own tests etc. in their folders. Each module exports const module: GraphQLSchemaModule = { typeDefs, resolvers } which are then glued together and passed to buildFederatedSchema.

However currently the GraphQLSchemaModule must be imported from apollo-graphql which is a dependency of @apollo/federation and we see little need in adding that as a direct dependency of our app.

This however triggers such an eslint warning for us:

image

Which is annoying as we have to ignore it all over the files. ( // eslint-disable-next-line import/no-extraneous-dependencies )

As GraphQLSchemaModule is type definition of an argument of buildFederatedSchema, it belongs to the public API of @apollo/federation, and this is why I think it should be exported by the @apollo/federation itself.

Thus, the PR.

@apollo-cla
Copy link

@jtomaszewski: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@trevor-scheer
Copy link
Member

Thanks for the PR @jtomaszewski - appreciate the explanation and this seems like a reasonable ask. This LGTM! Would you mind adding an entry to the federation-js/CHANGELOG.md?

@jtomaszewski
Copy link
Contributor Author

Done. When merging this, I suggest doing "Squash and merge", to merge the commits into one.

@trevor-scheer trevor-scheer merged commit d022be4 into apollographql:main Dec 15, 2020
@trevor-scheer
Copy link
Member

Done! Sorry for the delay on this @jtomaszewski. This change will go out with the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants