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

Switch ApolloServerBase.schema from private access to protected access. #1610

Merged
merged 4 commits into from Sep 5, 2018

Conversation

vincenzo
Copy link
Contributor

@vincenzo vincenzo commented Sep 4, 2018

I am proposing this change after a scenario I came across. I essentially wanted to build my class that extends ApolloServer, and mimic the built-in support for graphql-middleware that graphql-yoga implements.

My first implementation of that was done quickly in pure JS for Node (so, no TS) to explore the concept. In that case, of course, you can access anything in an object, so it was easy to do things like:

// options comes from a constructor
...
const { applyMiddleware } = require("graphql-middleware");
...
const server = new ApolloServer(options);
server.schema = applyMiddleware(server.schema, ...options.middlewares);
return server;

However, when I decided I wanted to refine this and make it a package, I switched to TypeScript.

So I had my class extending ApolloServer and my config extending Config.
And that's when I found out that I couldn't access this.schema to apply the graphql-middlewares after invoking the constructor of the superclass.

Whilst I could work around that by building an executable schema that I then apply the middlewares to before passing it to the ApolloServer constructor, that defeats the whole point of extending the base class, because I will have to duplicate a good portion of code in ApolloServerBase around the setting up of the executable schema.

I think it's fine for schema to be protected, and it will open up more possibilities for the subclasses.

TODO:

  • Update CHANGELOG.md with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

@ghost ghost added the ⛲️ feature New addition or enhancement to existing solutions label Sep 4, 2018
@apollo-cla
Copy link

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

@vincenzo vincenzo changed the title Switch ApolloServerBase.schema from private access to protected access. Switch ApolloServerBase.schema from private access to protected access. Sep 4, 2018
@ghost ghost added the ⛲️ feature New addition or enhancement to existing solutions label Sep 4, 2018
@martijnwalraven martijnwalraven merged commit 88417b7 into apollographql:master Sep 5, 2018
@vincenzo
Copy link
Contributor Author

vincenzo commented Sep 5, 2018

Thanks @martijnwalraven, I appreciate the merge! May I ask if you have an ETA for the next release that will include this? It'll unblock my work :)

@martijnwalraven
Copy link
Contributor

@vincenzo I just published a new release with this PR included :)

@vincenzo
Copy link
Contributor Author

vincenzo commented Sep 6, 2018

@martijnwalraven I can only see a new release for apollo-server -- does that include apollo-server-core too?

@vincenzo
Copy link
Contributor Author

vincenzo commented Sep 6, 2018

Forget that, they were all released 17 hours ago, but they have different versions, so I got confused :D

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
⛲️ feature New addition or enhancement to existing solutions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants