Skip to content

Conversation

@atierian
Copy link
Contributor

@atierian atierian commented Sep 24, 2024

Description of changes

Note

No functional changes

  • moves string defined JS resolvers to JavaScript file templates using [[KEY]] marker for template substitution.
  • adds copy-js-resolver-templates to move JS templates to /lib as part of build script.

Why use [[KEY]] as the marker?
This marker format satisfies two key goals:

  1. Minimal risk of overwriting previously substituted values, e.g. the value of [[KEY1]] containing "[[KEY2]]".
  2. Not break syntax highlighting in the JS file templates. The motivation behind moving to file templates is to improve maintainability of the resolver functions, for which syntax highlighting plays a vital role.

I'm satisfied that [[KEY]] is the appropriate format here. That said if you have an alternative in mind that satisfies both 1. and 2., and has other benefits, I'm happy to consider it.

CDK / CloudFormation Parameters Changed

N/A

Issue #, if available

N/A

Description of how you validated changes

  • CI PR workflow
  • locally run generation E2E tests

Checklist

  • PR description included
  • yarn test passes
  • E2E test run linked
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies
  • Any CDK or CloudFormation parameter changes are called out explicitly

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

@atierian atierian marked this pull request as ready for review September 24, 2024 21:05
@atierian atierian requested a review from a team as a code owner September 24, 2024 21:05
palpatim
palpatim previously approved these changes Sep 26, 2024
@atierian atierian merged commit 0c1aae5 into main Sep 27, 2024
5 checks passed
@atierian atierian deleted the ai.generation-js-resolver-templates branch September 27, 2024 04: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