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: Sync codegen behavior implementation adding generateModelsSync #894

Merged
merged 12 commits into from
Oct 23, 2024

Conversation

stocaaro
Copy link
Member

@stocaaro stocaaro commented Oct 10, 2024

Description of changes

This change translates the vendored code for codegen, adapting it so that it isn't async. Steps to adapt the code have been made as simple as possible where complexity is captured in related code to make upgrades as easy as possible as we maintain this. Directions are documented.

  • Document and apply the MAINTENANCE.md adaptations to the @graphql-codegen/core fork
  • Add a bunch of derived types to support sync codegen
  • Add sync preset/plugin implementation
  • Fix test errors caused by how jest hoisting works with mocks

Suggested review pattern:

  1. Copy the @graphql-codegen/core file across
    1. This change adds the version checking and copies across an unmodified version of @graphql-codegen/core.
    2. The code in the vendor folder will be an exact copy in this diff.
  2. Modify libraries to expose codegenSync behavior
    1. This diff covers all of the work to:
      1. Modify the vendored core code to be sync
      2. Add sync preset/plugin code to the plugins package
      3. Adds the sync types to the plugins package
      4. Add the generateModelsSync implementation
      5. Add CODEOWNERS requirement to get admin review for vendor changes

The copied code has been modified as little as possible and it is not intended to conform to local repo patterns as this would make future maintenance more difficult

Codegen Paramaters Changed or Added

const c = codegenSync(...);
// which is the same as without any async behavior
// const c = await codegen(...);
//
// Used to implement:
generateModelsSync(...)
// which is a sync version of
// generateModels(...)

generateModelsSync implementation

Description of how you validated changes

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Changes are tested using sample applications for all relevant platforms (iOS/android/flutter/Javascript) that use the feature added/modified
  • Changes are tested on windows. Some Node functions (such as path) behave differently on windows.
  • Changes adhere to the GraphQL Spec and supports the GraphQL types type, input, enum, interface, union and scalar types.

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

@stocaaro stocaaro changed the title feat: Sync codegen behavior implementation - modify and integrate feat: Sync codegen behavior implementation adding generateModelsSync Oct 10, 2024
@stocaaro stocaaro marked this pull request as ready for review October 21, 2024 20:45
@stocaaro stocaaro requested review from a team as code owners October 21, 2024 20:45
@@ -25,7 +25,7 @@
"@aws-amplify/graphql-docs-generator": "4.2.1",
"@aws-amplify/graphql-generator": "0.4.6",
"@aws-amplify/graphql-types-generator": "3.6.0",
"@graphql-codegen/core": "2.6.6",
Copy link
Member Author

Choose a reason for hiding this comment

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

Without this change, our code depends on 2 versions of core. Applying this so that we only have a single version in our dependency list for the monorepo

@@ -17,18 +17,23 @@
"aws"
],
"scripts": {
"build": "tsc",
"build": "tsc && yarn check-codegen-version",
Copy link
Member Author

Choose a reason for hiding this comment

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

Including the version check in the build process. As discussed, this will push the developer to look at the maintenance instructions if the version from npm is different than the one adapted in the vendor folder.

@stocaaro stocaaro merged commit fac63c1 into main Oct 23, 2024
4 checks passed
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.

4 participants