AppsyncFunction Interface

[sho-ai-main]

I love this project and I've been trying to use it in addition with graphql codegen to generate types automatically from the schema. While trying to do this I've had issues with the API for the AppsyncFunction class

The issue I see is that the name of the function parameter influences the result of the mapping template so if I use the code from the example app
This works

    this.getPerson = new AppsyncFunction<
      (id: string) => ProcessedPerson | null
    >((_$context, id) => {
      const person = this.personTable.getItem({
        key: {
          id: $util.dynamodb.toDynamoDB(id),
        },
        consistentRead: true,
      });

But this doesn't work because the name of the parameter is not the same as the name of the graphql query parameter

    this.getPerson = new AppsyncFunction<
      (id: string) => ProcessedPerson | null
    >((_$context, userId) => { // notice the argument name has changed
      const person = this.personTable.getItem({
        key: {
          id: $util.dynamodb.toDynamoDB(userId),
        },
        consistentRead: true,
      });

This means that no amount of code generation will help here as even if we successfully generate a type that looks like (id: string) => ProcessedPerson | null but don't use the right parameter name then you will not see a typescript error there.

Ideally I think that the interface for this function could look something like

AppsyncFunction<TArgs, TResult, TSource = undefined> {
    readonly decl: FunctionDecl<F>;
    constructor(fn: ($context: $Context<Source>, args: TArgs) => TResult);

Then the arguments would be used with args.id so slightly less good looking but with destructuring the difference is small, but this has the benefit of better type safety as using args.userId would result in a typescript error.

As an added benefit this would make using functionless with graphql codegen typescript resolver plugin much easier

[mathisobadia]

So I posted this from the wrong GitHub account 😂 let me know what you think about this. I could try to create a PR if needed!

[sam-goodwin]

Hey! I'm glad you like the project and thanks for the feedback.

One of the design tenets with Functionless is to try and map AWS services directly into native TypeScript syntax, which is why I was trying to represent $context.arguments.id as (id: string). I thought about args.id but the problem is the names cannot be captured at the type-level from a function signature:

type Func = (id: string) => Person

So, we would have to use an object to represent the args instead, like you're suggesting:

new AppsyncFunction(
  ($context: $Context<Source>, args: { id: string }): Person | undefined => {
    // blah
  })

If we go this route, then we can simplify things even further by placing args back in the $context object:

new AppsyncFunction(($context: $Context<Args, Source>) => {
  const userId = $context.arguments.id;
});

This may be even better since it matches the underlying Appsync environment exactly 1:1. https://docs.aws.amazon.com/appsync/latest/devguide/resolver-context-reference.html. Would it sill make it easy to integrate with the graphql codegen plugin?

RE: the bug when the argument name doesn't match the signature:

((_$context, userId) => { // notice the argument name has changed

At the very least, we can fix this in the typescript plugin by allowing developers to name those args whatever they please and mapping them back to the signature's argument name.

Thoughts?

[sho-ai-main]

I really like the solution of including the arguments as an object in the context to match the VTL template.

To make integration with graphql codegen easy what we need is to make the function accept an object with arguments as properties. It gives you TArgs, TResult, TSource for each resolver.
TArgs looks like {arg1: type, arg2: type}
And you can then create a type with those generic parameters so we can for example generate a resolver type that would be (args: TArgs) => TResult for each resolver function.

I spent some time trying to come up with a generic type that transforms the type (args: {arg1: type, arg2: type}) => TResult into (arg1: type, arg2: type) => TResult but couldn't really get it to work well.

I'm not sure what I'm saying is very clear (I can share the repo where I tried this maybe if that's helpful) but to answer the question first solution would make it easy to integrate with graphql codegen.

[sam-goodwin]

I'm always open to seeing a repo, so if you're ok sharing, please do.

I'm also convinced of the first solution. Functionless should always uphold the tenet of complying 1;1 with the underlying VTL environment. I'll work on a PR for this.

[sam-goodwin]

This will also fix #25

[sam-goodwin]

Still working on the PR, but this is what an AppsyncFunction is looking like now:

this.updateName = new AppsyncFunction(
  ($context: $Context<{ id: string; name: string }>) =>
    this.personTable.updateItem({
      key: {
        id: $util.dynamodb.toDynamoDB($context.arguments.id),
      },
      update: {
        expression: "SET #name = :name",
        expressionNames: {
          "#name": "name",
        },
        expressionValues: {
          ":name": $util.dynamodb.toDynamoDB($context.arguments.name),
        },
      },
    })
);

I like how we can now infer the types instead of declaring them in AppsyncFunction<(id: string) => Person)>. Both are possible, which is a huge improvement.

Alternative with explicit type:

this.updateName = new AppsyncFunction<{id: string, name: string}, Person>(
  ($context) =>
    this.personTable.updateItem({
      key: {
        id: $util.dynamodb.toDynamoDB($context.arguments.id),
      },
      update: {
        expression: "SET #name = :name",
        expressionNames: {
          "#name": "name",
        },
        expressionValues: {
          ":name": $util.dynamodb.toDynamoDB($context.arguments.name),
        },
      },
    })
);

TSource is the last type argument and defaults to undefined.

• I wonder if $Context is bad form and I should rename it to AppsyncContext?

[sam-goodwin]

Since this is a breaking change, I propose we also rename AppsyncFunction to AppsyncResolver. I mentioned some reasons why here: #7 (comment)

• rename AppsyncFunction to AppsyncResolver

[sam-goodwin]

@sho-ai-main @mathisobadia - PR is open, would love your feedback. #27

[mathisobadia]

@sam-goodwin I wrote some feedback on the PR, I think overall it's a great improvement and make everything more consistent with appsync terminology.

[mathisobadia]

I just tested this PR locally to add graphql codegen, you can check what this looks like here:
https://github.com/mathisobadia/functionless/blob/fix-context-arguments/test-app/src/people-db.ts

you get lots of

new AppsyncResolver<
      MutationResolvers["addPerson"]["args"],
      MutationResolvers["addPerson"]["result"],
      MutationResolvers["addPerson"]["parent"]
    >

but I could create a wrapper now without issues and everything works well already!

[sam-goodwin]

This is awesome!! Thanks for doing that.

We should provide some documentation on how to do this in the readme and update the example to use it.

Is it possible to add the wrapper class without taking a dependency on the graphql tools? Maybe that doesn't matter?

[mathisobadia]

@sam-goodwin I can open a pull request to do those changes if you want! I just tried to create the wrapper but can an error I don't totally understand when I try to deploy the stack (full code here ):

The wrapper is

type ResolverBase = {
  args: ResolverArguments;
  parent: unknown;
  result: unknown;
};

class AppsyncResolverWrapper<
  ResolverType extends ResolverBase
> extends AppsyncResolver<
  ResolverType["args"],
  ResolverType["result"],
  ResolverType["parent"]
> {
  constructor(
    fn: ResolverFunction<
      ResolverType["args"],
      ResolverType["result"],
      ResolverType["parent"]
    >
  ) {
    super(fn);
  }
}

And the error I get when deploying is

/Users/mathisobadia/sho/functionless/test-app/node_modules/functionless/src/appsync.ts:284
      return decl.body.statements.filter(
                       ^
TypeError: Cannot read properties of undefined (reading 'statements')
    at countResolvers (/Users/mathisobadia/sho/functionless/test-app/node_modules/functionless/src/appsync.ts:284:24)
    at AppsyncResolverWrapper.addResolver (/Users/mathisobadia/sho/functionless/test-app/node_modules/functionless/src/appsync.ts:131:27)
    at Object.<anonymous> (/Users/mathisobadia/sho/functionless/test-app/src/app.ts:28:20)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)
    at Module.m._compile (/Users/mathisobadia/sho/functionless/test-app/node_modules/ts-node/src/index.ts:1459:23)
    at Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Object.require.extensions.<computed> [as .ts] (/Users/mathisobadia/sho/functionless/test-app/node_modules/ts-node/src/index.ts:1462:12)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
Subprocess exited with error 1
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

I still don't understand fully the code so I don't know where to look to fix this.

Anyway I can still open the pull request with the previous version without a wrapper as that's working and the difference is not that big! let me know what you think.

[sam-goodwin]

Ah, that's because the typescript plug-in is not transforming the function into its AST form. The compiler looks for the AppsyncResolver class or the reflect function and converts the function into an object.

We will have to update this code to support classes that extend AppsyncResolver.

https://github.com/sam-goodwin/functionless/blob/be8d032c6e0f58658854e3bbc15901553a62a2c6/src/compile.ts#L84

[sam-goodwin]

Opened an issue to track this and provided some more information #30

[sam-goodwin]

Also opened an issue to track the wrapper class specifically: #31