-
Notifications
You must be signed in to change notification settings - Fork 651
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
[apollo-execution] Allow to pass arguments to the root types #5352
Conversation
f5d1d55
to
a6ff559
Compare
@@ -23,6 +23,9 @@ | |||
<Properties> | |||
<option name="KEEP_BLANK_LINES" value="true" /> | |||
</Properties> | |||
<ScalaCodeStyleSettings> | |||
<option name="MULTILINE_STRING_CLOSING_QUOTES_ON_NEW_LINE" value="true" /> | |||
</ScalaCodeStyleSettings> |
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.
Not sure why this started happening. This is for scala so I do not expect too much impact. If this ends up moving too much, I'll dig into it.
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.
Very cool! 🎉
✅ Deploy Preview for apollo-android-docs canceled.
|
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't approve my own PR but LGTM!
if (apolloClients.containsValue(id)) error("Name '$id' already registered") | ||
apolloClients[apolloClient] = id | ||
startOrStopServer() | ||
lock.withLock { |
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 don't think that works? This locks the writer but readers from the HTTP request thread won't use the lock and might iterate the list while it is modified
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.
Ah you're right! I liked the AtomicReference
idea but didn't like that it's JVM only. But maybe that's ok.
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.
AtomicFu has KMP versions.
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.
Ah great! 🎉
I think I'll use JVM AtomicReference
to avoid adding the AtomicFu dependency, but with the knowledge that we can use it to support more targets.
Add a
${ServiceName}ExecutableSchemaBuilder()
constructor function that sets up a pre-configuredExecutableSchema.Builder
.This function takes a
() -> QueryType
, etc.. for root types so it's easier to pass arguments to the root instances.