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

GQLGW-2564: Static Args Work #461

Merged
merged 15 commits into from
Oct 11, 2023
Merged

GQLGW-2564: Static Args Work #461

merged 15 commits into from
Oct 11, 2023

Conversation

sbarker2
Copy link
Collaborator

@sbarker2 sbarker2 commented Sep 14, 2023

Hydrated input syntax is currently required to be passed as a variable (e.g $source.varName or $argument.varName). we want to implement static args, like so:

arguments : [
{ name:"type" value:"some-static-argument-type"},

Note that this could be for many different types (e.g. String, Boolean, Int, Float, Array, Object).

@sbarker2 sbarker2 requested a review from gnawf September 14, 2023 21:48
@sbarker2 sbarker2 changed the title [WIP] Static args Static Args Sep 25, 2023
@sbarker2 sbarker2 changed the title Static Args GQLGW-2564: Static Args Work Sep 25, 2023
actorFieldArg to staticValue
} else {
null
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it ended up null - should it be a map of name to null?

Why does this makeNormalizedInputValue methods return null is really the question - is it a I could not handle this

like can you do [{name: "someAttribute" value: null}] say ??

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So looking into it, this function doesn't actually return null. This syntax was just a result of copying the ArgumentValue case above. I will update now

Copy link
Collaborator Author

@sbarker2 sbarker2 Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this makeNormalizedInputValue methods return null is really the question

so the answer is actually it doesn't. Good catch!

@@ -72,7 +75,7 @@ object NadelDirectives {
)
.inputValueDefinition(
name = "value",
type = nonNull(GraphQLString),
type = nonNull(ExtendedScalars.Json),

This comment was marked as resolved.

Copy link
Collaborator Author

@sbarker2 sbarker2 Oct 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, this used to only have the options "$source.blah" or "$args.blah" which were always a string.
We only need it now because the static types could be other types (mentioned in description of PR)

// todo this should be a union or sealed class thing
data class RemoteArgumentSource(
val argumentName: String?, // for OBJECT_FIELD
val pathToField: List<String>?,
val sourceType: SourceType?,
val staticValue: Value<*>?,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we dont have to do it for this PR - but I think this well become just astValue when we do the mix or static or dynamic work later

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good

Copy link
Collaborator

@gnawf gnawf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments, of which 99% of them should have a suggestion you can just commit directly to resolve.

Looking good.

@sbarker2 sbarker2 requested a review from gnawf October 10, 2023 01:46
Copy link
Collaborator

@gnawf gnawf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

NadelHydrationActorInputDef.ValueSource.StaticValue(
value = remoteArgDef.remoteArgumentSource.staticValue!!
)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again later the naming here is likely to be AstArgument and NadelHydrationActorInputDef.ValueSource.AstValue

@sbarker2 sbarker2 merged commit 9ccdd45 into master Oct 11, 2023
2 checks passed
@sbarker2 sbarker2 removed the request for review from faizan-siddiqui October 11, 2023 05:14
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