feat(appsync): remove addResolver from AppsyncResolver and lazy template evaluation #275

sam-goodwin · 2022-06-18T08:53:11Z

Fixes #137
Fixes #65

AppsyncResolver is now a Construct that takes as props the api, typeName, fieldName and resolve function (Remove addResolver utility from AppsyncResolver #137)

export const getDeletionStatus = new AppsyncResolver<
  { executionArn: string },
  string | undefined
>(stack, "getDeletionStatus", {
  api,
  typeName: "Query",
  fieldName: "getDeletionStatus",
  resolve: ($context) => {
    const executionStatus = deleteWorkflow.describeExecution(
      $context.arguments.executionArn
    );

    return executionStatus.status;
  },
});

Lazily evaluate VTL interpreters so that fields can be defined in any order (AppSync: addResolver breaks if called before dependency is defined #65)
Move addField to its own class and also apply lazy evaluation

BREAKING CHANGE: AppsyncResolver is now a Construct and creates the resolvers within its constructor

…removed. Templates are eagerly evaluated so that order can be arbitrary.

netlify · 2022-06-18T08:53:18Z

✅ Deploy Preview for effortless-malabi-1c3e77 ready!

Name	Link
🔨 Latest commit	`e4d7105`
🔍 Latest deploy log	https://app.netlify.com/sites/effortless-malabi-1c3e77/deploys/62b0fc76ac61c000090abd56
😎 Deploy Preview	https://deploy-preview-275--effortless-malabi-1c3e77.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

src/appsync.ts

src/compile.ts

src/util.ts

test-app/src/message-board.ts

src/appsync.ts

test-app/src/message-board.ts

… class

src/appsync.ts

…I instead

thantos · 2022-06-20T12:59:30Z

src/appsync.ts

 * @functionless AppsyncFunction
 */
 export class AppsyncResolver<
  Arguments extends ResolverArguments,
  Result,
  Source = undefined
-> {
+> extends Construct {


Didn't we just make the StepFunction not a Construct/Resource? Maybe AppSync should wrap and not be too? What is the value of it itself being a construct? Will we ever want to call an app sync resolver from lambda?

Not sure we would ever call a resolver directly. I don't think it's even possible? We can execute graphql queries but not individual resolvers.

I guess if there is value in this being a construct, it is fine.

I'm also fine with moving it to resource. Keep things consistent.

thantos · 2022-06-20T13:00:48Z

src/appsync.ts

+
+    this.resolvers = memoize(() => synthResolvers(props.api, this.decl));
+
+    this.resource = new aws_appsync.CfnResolver(this, "Resource", {


why CfnResolver and not he L2?

Because the L2 doesn't expose IResolvable which I need to support lazy evaluation. Without lazy evaluation, users are forced to order things eagerly.

Makes sense, I think we'll need to do this in other places.

thantos · 2022-06-20T13:03:50Z

src/appsync.ts

+  if (resolverCount === 0) {
+    if (pipelineConfig.length !== 0) {
+      throw new Error(


In my PR I removed this check. I am not sure I understand the purpose, do we think we'll mess up? Couldn't an integration provide only vtl in the future?

Yeah, I don't really know what this is checking for either. Seems to check for a situation where we rendered a pipeline without ever calling a resolver? Seems like internal bug.

Which PR should we merge first?

thantos · 2022-06-20T13:05:15Z

src/appsync.ts

+              throw new Error(
+                `invalid Expression in-lined with Service Call: ${expr.kind}`
+              );


Synth error, also I don't understand how this happens?

I haven't changed this code. It is a bit hacky - it is trying to support various different ways in which the result of an Integration call may be referenced.

E.g.

call().result call().result[0]

This will be fixed with your a-normal form change? #280

Ohh, yeah, I actually removed this whole function, lol

in the async change.

thantos · 2022-06-20T13:14:35Z

src/appsync.ts

+  constructor(
+    options: AppsyncFieldOptions,
+    resolve: ResolverFunction<Arguments, Result, Source>
+  ) {


I was trying to remember why field and resolver are different.

Ha...

aws/aws-cdk#18351 (comment)

I don't like how buggy it is, ran into this again when working on the authorizer example.

Can you add a validator or something that checks to see if a resolver was actually created?

Do you mean a node validator? There is no Construct here for me to add that validator to. Also, if the resolver isn't added, then the validator also wouldn't be added? Is this solved with just tests?

Hmm, you could put it on the api.

api.node.addValidator(() => { api.node.tryFindChild(`${props.typeName}${props.fieldName}Resolver`) ? [] : ["uhh ohh no resolver!"] });

I don't have a typeName or fieldName at this point. That happens later when you call addQuery. I don't think there's anything to validate here?

thantos · 2022-06-20T13:19:50Z

test/__snapshots__/vtl.test.ts.snap

-  \\"version\\": \\"2018-05-29\\",
-  \\"payload\\": null
+\\"version\\": \\"2018-05-29\\",
+\\"payload\\": null


You meant to do this?

I don't recall changing the vtl logic

I think it was a literal string change.

thantos · 2022-06-20T13:20:55Z

test/test-files/appsync.ts

+new AppsyncResolver(
+  stack,
+  "getPost",
+  {
+    api,
+    typeName: "Query",
+    fieldName: "getPost",
+  },
+  // valid - inline arrow function
+  () => null
+);


Love these tests.

They do get stuck periodically though

Hmm that's odd, any idea why that would be? I've never seen that happen.

I am not sure, never get an error, just spins. Running it on its own works fine, maybe a race condition?

feat(appsync): AppsyncResolver is now a Construct and addResolver is …

47a519e

…removed. Templates are eagerly evaluated so that order can be arbitrary.

sam-goodwin self-assigned this Jun 18, 2022

sam-goodwin requested a review from thantos June 18, 2022 08:53

sam-goodwin added the appsync label Jun 18, 2022

thantos reviewed Jun 18, 2022

View reviewed changes

src/appsync.ts Outdated Show resolved Hide resolved

thantos requested changes Jun 18, 2022

View reviewed changes

fix: pipelineConfig to use ARNs and move addField out to AppsyncField…

87d46b8

… class

sam-goodwin marked this pull request as ready for review June 19, 2022 22:59

sam-goodwin requested a review from thantos June 19, 2022 23:01

sam-goodwin commented Jun 19, 2022

View reviewed changes

src/appsync.ts Outdated Show resolved Hide resolved

chore: update docs and website docs

9a12573

thantos mentioned this pull request Jun 20, 2022

fix: appsync and step function visitor hoist refactor #280

Merged

feat: allow a GraphQLApi to be passed as scope and use that as the AP…

78c95da

…I instead

thantos requested changes Jun 20, 2022

View reviewed changes

sam added 3 commits June 20, 2022 15:46

Merge branch 'main' into appsync-remove-add-resolver

a764911

chore: cdk synth --quiet

ae2aee2

feat: remove Construct as base type of AppsyncResolver

e4d7105

thantos approved these changes Jun 20, 2022

View reviewed changes

sam-goodwin merged commit e1dcd83 into main Jun 20, 2022

sam-goodwin deleted the appsync-remove-add-resolver branch June 20, 2022 23:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(appsync): remove addResolver from AppsyncResolver and lazy template evaluation #275

feat(appsync): remove addResolver from AppsyncResolver and lazy template evaluation #275

sam-goodwin commented Jun 18, 2022 •

edited

netlify bot commented Jun 18, 2022 •

edited

thantos Jun 20, 2022

sam-goodwin Jun 20, 2022 •

edited

thantos Jun 20, 2022

sam-goodwin Jun 20, 2022

thantos Jun 20, 2022

sam-goodwin Jun 20, 2022

thantos Jun 20, 2022

thantos Jun 20, 2022

sam-goodwin Jun 20, 2022

sam-goodwin Jun 20, 2022

thantos Jun 20, 2022

sam-goodwin Jun 20, 2022

thantos Jun 20, 2022

thantos Jun 20, 2022

thantos Jun 20, 2022

sam-goodwin Jun 20, 2022

thantos Jun 20, 2022

sam-goodwin Jun 20, 2022

thantos Jun 20, 2022

sam-goodwin Jun 20, 2022

thantos Jun 20, 2022

thantos Jun 20, 2022

sam-goodwin Jun 20, 2022

thantos Jun 20, 2022


		this.resolvers = memoize(() => synthResolvers(props.api, this.decl));

		this.resource = new aws_appsync.CfnResolver(this, "Resource", {

feat(appsync): remove addResolver from AppsyncResolver and lazy template evaluation #275

feat(appsync): remove addResolver from AppsyncResolver and lazy template evaluation #275

Conversation

sam-goodwin commented Jun 18, 2022 • edited

netlify bot commented Jun 18, 2022 • edited

✅ Deploy Preview for effortless-malabi-1c3e77 ready!

Choose a reason for hiding this comment

sam-goodwin Jun 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-goodwin commented Jun 18, 2022 •

edited

netlify bot commented Jun 18, 2022 •

edited

sam-goodwin Jun 20, 2022 •

edited