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.
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
feat(appsync): import existing graphql api #9254
feat(appsync): import existing graphql api #9254
Changes from 8 commits
1e0fea7
748b1c2
7b77d43
fc12e14
64b4f98
3bde2e9
48f7824
a68e1f6
b0ea1a2
d1e7c48
e136805
07dfea2
142c5f5
7e9b775
a44a9b8
d7678ba
c066ba9
ecc9145
4976bab
5243ef0
a668e13
5a15cf0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
it feels like the signature should be using our pattern of
...Options
as an input parameter. Maybe something likeaddDynamoDbDataSource(name: string, options: DynamoDbDataSourceOptions)
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.
Why is that? there aren't that many configurable parameters so i'm not sure if an
options
prop is necessaryThere 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.
it's consistent with the rest of the construct library and also compliant with design guidelines
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.
clarify - is this the depender or the dependee
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.
construct is the dependee
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 provide a more helpful description.
what happens if this method is called and it is imported? it's unclear what the implications are if it is imported
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.
did we want to give this a specific prefix before the
arn
- since we're introducing this, let's make sure it aligns with how we define arn elsewhereThere 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.
if we change this here i feel like we need to change it for every where else.. including the GraphQLAPI.. i think this breaking change should happen in another pr
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.
curious about the casing here (and everywhere really)... how does this translate to Python where we do snake casing?
does
GraphQL
get broken down intograph_q_l
?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.
yeah i think in this case would be best to change all of them to graphqlUrl for props
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.
^ this would be a breaking change tho so not sure if thats the best move for something so small..
@MrArnoldPalmer thoughts?
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 not sure it needs to happen in this PR, but fixing the casing sounds like something we need to do regardless if it requires code that is not idiomatic for Python developers
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.
yeah i think a chore might be better
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.
do we need this method? it seems like it's substituting one API call (
addDependsOn
) with another (addSchemaDependency
). any reason users can't just do it directly?does it require validation? what's valid as a construct to add
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.
there needs to be a dependency add for resolvers and the schema, but i was not comfortable passing schema's around in imports so essentially i needed a way to allow for adding a dependency only if the api wasn't imported.
this basically was my workaround.. if anything i can make this function protected
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 make it internal?
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've thought about this a little more and I think that it might be good to offer a public api to add a dependency.. if the graphql api is in another stack, it will be deployed before the imported stack so the dependency doesn't matter too much?
but in the case that it does we should have a public api to allow for adding dependencies? if not i dont see why a public api is a net bad
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.
if it enables use cases that users cannot achieve, then I agree.
this method calls
construct.addDependsOn(schema)
and returns true. can users just make that api call instead?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.
@shivlaks its just really annoying for users to have to do after creating each resolver.
The only reason its public as of right now is that I need
IGraphQLApi
to have some capability to reference a schema, I can't make it protected or the function will be inaccessible to the Resolver class.