-
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
Make ApolloServer.stop invoke ApolloGateway.stop #4907
Merged
Merged
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Nit: I don't think this needs to be
await
ed here since thetoDispose
array is wrapped in anawait
edPromise.all
in the server'sstop
fn. Also added an optional chain in my suggestion.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.
Ooh, I totally forgot about
?.()
and was confused about why I couldn't getgateway?.stop()
to work.It does still need to be
gateway?.stop?.()
because I addedstop
as an optional method on theGraphQLService
interface ... just in case there is somebody out there who for some reason has a non-@apollo/gateway
implementation ofGraphQLService
. (I think in AS3 we should get rid of that abstraction and just have gateway be ApolloGateway.)I do actually find
async () => await gateway?.stop?.()
to be clearer than() => gateway?.stop?.()
in that it makes it really obvious that stop is an async function. Plenty of stop functions exist in the Node world that aren't async, and so to me it adds some clarity. Is that weird of me?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 guess you told me the same thing last year and I ignored you that time. #4450 (comment)
Though in that one there was some extra complexity around Promise.all and typing that made dropping the async/await not work trivially.
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.
You'd only need the first ?
gateway?
ifgateway
was possibly nullish, but we know here that it isn't. It's only thestop
fn on thegateway
object that might not exist so just the one is sufficientasync () => await gateway.stop?.()
.And that's a fine reason to me to keep it around if you prefer!
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.
Actually... I'm continuing to find myself uncomfortable with functions that return
ValueOrPromise<void>
. After all "void" means "I'm not going to look at the return value" which is sorta incompatible with "but I need to see if it's a Promise or not". When it's in our public API then we're kinda stuck with it, but toDispose is entirely private, so I'd like to just make itSet<() => Promise<void>>
by making the one place where we have a sync function just not be sync.And once you do that, you actually do need the async/await here, since in the "there is no
stop
" case it's not returning a Promise.Also completely ignore my comment above about needing a question mark after
gateway
, that was wrong.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.
Aren't we only stuck with it as far as compile-time typings go? Might we consider changing that typing in our public API within v3? If so, let's make a ticket and put it on the milestone?
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.
Yes, though I would really just be tempted to eliminate GraphQLService and change the parameter to
gateway?: ApolloGateway
too. (Or even just add "executeOperation" and "loadSchema" hooks to the plugin interface and make gateway into a plugin.) #4919