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(amplify-codegen): add schema compile process in codegen commands #5164

Merged
merged 5 commits into from
Oct 19, 2020

Conversation

AaronZyLee
Copy link
Contributor

@AaronZyLee AaronZyLee commented Aug 24, 2020

Issue #, if available:
fix #4882
Description of changes:
Added graphql schema compiling in command amplify codegen, amplify codegen statements and amplify codegen types.

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

@AaronZyLee AaronZyLee changed the title fix: add schema compile process in codegen commands feat: add schema compile process in codegen commands Aug 24, 2020
@AaronZyLee AaronZyLee changed the title feat: add schema compile process in codegen commands feat(amplify-codegen): add schema compile process in codegen commands Aug 24, 2020
Comment on lines 50 to 53
await context.amplify.executeProviderUtils(context, 'awscloudformation', 'compileSchema', {
forceCompile: true,
});

Copy link
Contributor

Choose a reason for hiding this comment

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

We might need to add a check here to ensure that there is an valid GraphQL API in the project before executing this. The CLI allows customers to add external api using amplify add codegen --apiId <your-api-id>. If the API is an external API, then we download the introspection schema and generate types and statements.

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.

Please add a check to ensure there is an GraphQL API provisioned in the project before executing compileSchema

await context.amplify.executeProviderUtils(context, 'awscloudformation', 'compileSchema', {
forceCompile: true,
});
if (!project.schema.endsWith('.json') && fs.existsSync(path.join(projectPath, project.schema))){
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer the check rely on existence of a API resource along with looking at the file extension. There is a possibility that the API is removed, but the .graphql.yml file is not updated. You could follow something similar to what we do in mock. Also move the check to an helper method instead of having the same check in 3 different places

private async getAppSyncAPI(context) {
const currentMeta = await getAmplifyMeta(context);
const { api: apis = {} } = currentMeta;
let appSyncApi = null;
let name = null;
Object.entries(apis).some((entry: any) => {
if (entry[1].service === 'AppSync' && entry[1].providerPlugin === 'awscloudformation') {
appSyncApi = entry[1];
name = entry[0];
return true;
}
});
if (!name) {
throw new Error('No AppSync API is added to the project');
}
return name;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. I will add this part to the check

@lgtm-com
Copy link

lgtm-com bot commented Aug 27, 2020

This pull request introduces 1 alert when merging 975abd3 into f8ff81d - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@yuth yuth merged commit 85f739a into aws-amplify:master Oct 19, 2020
SwaySway added a commit that referenced this pull request Oct 26, 2020
SwaySway added a commit that referenced this pull request Oct 26, 2020
r0zar pushed a commit to r0zar/amplify-cli that referenced this pull request Nov 19, 2020
…aws-amplify#5164)

Updated codegen command to run GraphQL Schema compilation before generating the code so the generated code is reflecting the schema changes
r0zar pushed a commit to r0zar/amplify-cli that referenced this pull request Nov 19, 2020
@github-actions
Copy link

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants