-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add createGateway helper function #2911
Add createGateway helper function #2911
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really excited to move towards this API! I think we should wait on this PR until we also add in the needed functionality into AS that makes accepting a gateway possible. It may require larger scale changes and will most certainly be a removal of the existing API so we will need to update the docs.
Can you sync with @glasser and @abernix on next steps for changes internally to AS?
This is the thing that we had discussed in our conversation @jbaxleyiii . Here are some other brainstormy ideas that avoid the sorta inelegant "apollo-server has to know about gateway and it's not clear what it's doing with that data":
This is bad because it doesn't let you choose which ApolloServer implementation you want. But how about:
The things I don't like here are:
|
Or @JacksonKearl suggests something like:
where in this case, |
@jbaxleyiii AS can already accept a gateway via |
I personally don't see a big issue with passing const apiKey = getApiKeyFromSomewhereBesidesTheEnvironment()
const server = new ApolloServer({
gateway: await createGateway({apiKey}),
engine: {apiKey},
playground: process.env === 'STAGING'
}) or const apiKey = getApiKeyFromSomewhereBesidesTheEnvironment()
const server = new ApolloServer({
...await createGateway({apiKey}),
engine: {apiKey},
playground: process.env === 'STAGING',
}) or even ditch const apiKey = getApiKeyFromSomewhereBesidesTheEnvironment()
const server = new ApolloServer({
gateway: await new ApolloGateway({apiKey}).load(),
engine: {apiKey},
playground: process.env === 'STAGING'
}) and I wonder how many people aren't defining the |
If we decide it's OK to have people pass |
@glasser @JacksonKearl we will need support for an |
@jbaxleyiii one caveat is that you never actually need to destructure then pass. You can just pass the return value. This is made more formal by having the gateway creator return an interface and the server constructor accepting that interface. |
I find "we give it to you as an object of a class with methods but you're expected to use it by destructuring it" to be quite weird. I don't even feel like I know well as a developer on this project exactly which fields/methods from a class are going to "show up" when destructured and what won't. It's one thing to say " |
@@ -261,6 +263,19 @@ export class ApolloServerBase { | |||
throw new Error(errors.map(error => error.message).join('\n\n')); | |||
} | |||
this.schema = schema!; | |||
} else if (gateway) { | |||
this.schema = gateway.schema; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a throw
here if subscriptions are currently enabled? While there's an unlikely chance that some aspect of the (bolted on, but entirely separate) subscription server might startup and run right now with the current federation implementation, we're even farther from being able to support that when schema reloading comes into the picture.
While we're going to want to fix that, that's unlikely to be before Apollo Server 3 which hopes to fix subscriptions (See #2360).
@@ -77,7 +84,7 @@ export interface Config extends BaseConfig { | |||
introspection?: boolean; | |||
mocks?: boolean | IMocks; | |||
mockEntireSchema?: boolean; | |||
engine?: boolean | EngineReportingOptions<Context>; | |||
engine?: false | EngineReportingOptions<Context>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just true
is still a valid option, no?
Just going to merge this into the staging branch now because it is getting to be a big hassle to tie all these things together while everything is still partially implemented on separate branches. |
This is an attempt to implement @glasser's desired API for create gateway. As stated in the comments of AS-105: