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

feat(graphql-function-transformer): port @function to v2 #7055

Merged
merged 3 commits into from Apr 29, 2021
Merged

feat(graphql-function-transformer): port @function to v2 #7055

merged 3 commits into from Apr 29, 2021

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 10, 2021

Description of changes

This commit ports the @function directive to v2 of the GraphQL
Transformer.

Issue #, if available

Description of how you validated changes

Manual and automated testing.

Checklist

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@cjihrig cjihrig requested a review from yuth April 10, 2021 21:57
@cjihrig cjihrig requested a review from a team as a code owner April 10, 2021 21:57
@lgtm-com

This comment has been minimized.

@codecov
Copy link

codecov bot commented Apr 11, 2021

Codecov Report

Merging #7055 (8c721b2) into master (07525b3) will increase coverage by 0.09%.
The diff coverage is 98.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7055      +/-   ##
==========================================
+ Coverage   56.43%   56.52%   +0.09%     
==========================================
  Files         445      447       +2     
  Lines       21859    21909      +50     
  Branches     4373     4380       +7     
==========================================
+ Hits        12336    12385      +49     
- Misses       8745     8746       +1     
  Partials      778      778              
Impacted Files Coverage Δ
...on-transformer/src/graphql-function-transformer.ts 97.95% <97.95%> (ø)
.../amplify-graphql-function-transformer/src/index.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07525b3...8c721b2. Read the comment docs.

@renebrandel renebrandel added this to Review in Bug bash via automation Apr 23, 2021
Bug bash automation moved this from Review to Approved and ready to merge Apr 23, 2021
Copy link
Contributor

@yuth yuth left a comment

Choose a reason for hiding this comment

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

Overall looks good. Couple of nits. Onething we should think a bit deeper about is to see if directives like @auth or data enhancers say @default can be built in a generic way and if it can be used with the @function directive

return;
}

const stack: cdk.Stack = context.stackManager.createStack(FUNCTION_DIRECTIVE_STACK);
Copy link
Contributor

Choose a reason for hiding this comment

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

We have had some issues come in earlier where customers have hit the stack size limitation for directives that create just 1 stack (like connection). Since only one stack contains all the @function, could we look into splitting this when it hits some threshold?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to avoid character counting if possible. It would also be nice if the related data source, app sync function, and resolvers could be kept together in the same file to avoid awkward dependencies between files.

What do you think about splitting this up at the data source level? That would ensure that there are no dependencies across files. It would still be theoretically possible to hit a limit, but I don't think that can ever be avoided if an app is sufficiently large.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to avoid character counting if possible. It would also be nice if the related data source, app sync function, and resolvers could be kept together in the same file to avoid awkward dependencies between files.

I don't char counting either. One approach we could take is to say how many function we put in a single stack, and if we exceed the number of functions then we create another stack for function. I am not sure what would the ideal number of function in a stack. We could potentially change the number of function in future if we want without causing any issue as the functions are just data sources and by themself they can be moved around without causing any data loss.

What do you think about splitting this up at the data source level? That would ensure that there are no dependencies across files. It would still be theoretically possible to hit a limit, but I don't think that can ever be avoided if an app is sufficiently large.

I am not sure if I understand this correctly, but with @function the DataSource is a lambda function right. Are you suggesting all the @function that use the same lambda function go in a single stack?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting all the @function that use the same lambda function go in a single stack?

Yes. So, in the following example, there would be a single data source, a single appsync function configuration, and two pipeline resolvers. I was proposing trying to keep all of that together in a single file.

type Query {
    echo(msg: String): String @function(name: "echofunction-\${env}")
    magic(msg: String): String @function(name: "echofunction-\${env}")
}

One approach we could take is to say how many function we put in a single stack, and if we exceed the number of functions then we create another stack for function.

That would work too, and should be fairly straightforward.

This commit adds the DirectiveWrapper to GraphQL Transformer v2.
This commit ports the @function directive to v2 of the GraphQL
Transformer.
@attilah attilah merged commit 463e975 into aws-amplify:master Apr 29, 2021
Bug bash automation moved this from Approved and ready to merge to Done Apr 29, 2021
@cjihrig cjihrig deleted the func branch April 29, 2021 19:29
@siegerts
Copy link
Contributor

siegerts commented May 4, 2021

👋 Hi, this issue was referenced in the v4.50.2 release!

Check out the release notes here https://github.com/aws-amplify/amplify-cli/releases/tag/v4.50.2.

@siegerts siegerts added the referenced-in-release Issues referenced in a published release changelog label May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
referenced-in-release Issues referenced in a published release changelog
Projects
Bug bash
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants