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

(aws-lambda-nodejs): ICommandHooks requires all hooks to be implementated #13457

Open
mjwbenton opened this issue Mar 7, 2021 · 8 comments
Open
Labels
@aws-cdk/aws-lambda-nodejs bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@mjwbenton
Copy link

Reproduction Steps

    new lambda.NodejsFunction(this, "LambdaFunction", {
      bundling: {
        commandHooks: {
          afterBundling(inputDir: string, outputDir: string): string[] {
            return [`echo "hello world"`];
          },
        },
      },
    });

What did you expect to happen?

For this to be valid, and the equivalent of defining no commands for beforeBundling and beforeInstall.

What actually happened?

Type '{ afterBundling(inputDir: string, outputDir: string): string[]; }' is missing the following properties from type 'ICommandHooks': beforeBundling, beforeInstall

Environment

  • CDK CLI Version : 1.91.0
  • Framework Version: 1.91.0
  • Node.js Version: v14.15.1
  • OS : macOS Catalina
  • Language (Version): Typescript (4.2.3)

Other

Happy to provide a pull-request for this, looks very easy to fix. Unless I'm missing something we can just declare the functions as optional and then default them to the equivalent of () => [].


This is 🐛 Bug Report

@mjwbenton mjwbenton added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 7, 2021
@jogold
Copy link
Contributor

jogold commented Mar 8, 2021

@eladb this is #11583 (comment)

what do you thinK?

@mjwbenton
Copy link
Author

Ah yeah, it looks like they were made non-optional for jsii reasons then. I did wonder if that was what I was missing.

It's pretty strange from a user perspective, but I understand not wanting the experience to be different across different languages. It's not super important, so fair enough if you want to do nothing, but I would have been less likely to question it if maybe the example in the documentation showed...

  beforeBundling: () => []
  beforeInstall: () => []

rather than // ...

I'm not familiar with the ins and outs of JSII, and I'm only deeply familiar with Java when it comes to the other languages, but trying to think about ways around this.

If you were building this out natively in Java you'd likely define a functional interface for a hook function, something like...

interface CommandHook {
  List<String> hook(string inputDir, string outputDir);
}

Then you'd use lambdas that adhere to that interface as values, so that null becomes a possibility and therefore their implementation is optional. I'm guessing going down that path isn't possible with JSII? Are there other examples in the CDK where functions are passed as values?

The only other thing that I can think, that I assume must be compatible with JSII, is to wrap each function up in another object so that the whole of that can be optional in the way that commandHooks is optional today.

interface CommandHookFunction {
  run: (inputDir: string, outputDir: string) => string[]
}

interface CommandHooks {
  beforeBundling?: CommandHookFunction,
  afterBundling?: CommandHookFunction,
  beforeInstall?: CommandHookFunction
}

commandHooks: {
  beforeBundling: {
    run: (inputDir: string, outputDir: string) => [...]
  }
}

@eladb
Copy link
Contributor

eladb commented Mar 18, 2021

Yeah it's a limitation we have. We could split up the interface to multiple interfaces (one for each hook), and then make the props optional

@eladb eladb added p2 effort/small Small work item – less than a day of effort labels Mar 18, 2021
@eladb
Copy link
Contributor

eladb commented Mar 25, 2021

I am unassigning and marking this issue as p2 which means that we are unable to work on this immediately.

We use +1s to help us prioritize our work, and as always we are happy to take contributions if anyone is interested to pick this up and submit a PR (please make sure to follow our contribution guidelines.

🙏

@eladb eladb removed their assignment Mar 25, 2021
@ryparker ryparker removed the needs-triage This issue or PR still needs to be triaged. label Jun 1, 2021
@piotrekwitkowski
Copy link

A Python example how to use command_hooks would be greatly appreciated!

@icj217
Copy link

icj217 commented Apr 19, 2022

Would also appreciate a python example. I've tried a couple different permutations but all of them result in TypeError: Don't know how to convert object to JSON: <class 'helpers.bundling.CustomCommandHooks'> errors

Attempts:

from aws_cdk import (
    aws_lambda_nodejs as njs,
)
class CustomCommandHooks(njs.ICommandHooks):
    def before_bundling(self, input_dir: builtins.str, output_dir: builtins.str) -> typing.List[builtins.str]:
        return []

as well as:

@jsii.interface(jsii_type="aws-cdk-lib.aws_lambda_nodejs.ICommandHooks")
class CustomCommandHooks:

    @jsii.member(jsii_name="afterBundling")
    def after_bundling(self, input_dir: builtins.str, output_dir: builtins.str) -> typing.List[builtins.str]:
        return []

    @jsii.member(jsii_name="beforeBundling")
    def before_bundling(self, input_dir: builtins.str, output_dir: builtins.str) -> typing.List[builtins.str]:
        return [f"cd {input_dir}", "npm install"]

    @jsii.member(jsii_name="beforeInstall")
    def before_install(self, input_dir: builtins.str, output_dir: builtins.str) -> typing.List[builtins.str]:
        return []

Commenting out the @jsii.member method decorators yields same error.

Changing the class decorator to @jsii.implements(njs.ICommandHooks) in previous example yields AttributeError: type object 'type' has no attribute '__jsii_type__' error.

@icj217
Copy link

icj217 commented Apr 20, 2022

Was able to figure out how to implement this in Python. The key step was creating an instance of the class and passing that into BundlingOptions

import typing
import builtins
import jsii
from aws_cdk import (
    aws_lambda_nodejs as njs,
)
​
@jsii.implements(njs.ICommandHooks)
class CustomCommandHooks:
​
    @jsii.member(jsii_name="afterBundling")
    def after_bundling(self, input_dir: builtins.str, output_dir: builtins.str) -> typing.List[builtins.str]:
        return []
​
    @jsii.member(jsii_name="beforeBundling")
    def before_bundling(self, input_dir: builtins.str, output_dir: builtins.str) -> typing.List[builtins.str]:
        return [f"cd {input_dir}", "yarn"]
​
    @jsii.member(jsii_name="beforeInstall")
    def before_install(self, input_dir: builtins.str, output_dir: builtins.str) -> typing.List[builtins.str]:
        return []

...


hooks = CustomCommandHooks()
​
options = njs.BundlingOptions(
        source_map=True,
        target="es2020",
        format=njs.OutputFormat.CJS,
        loader={".yaml": "file"},
        command_hooks=hooks)

@piotrekwitkowski FYI

@madeline-k
Copy link
Contributor

There is also a basic example in the README for this module: https://docs.aws.amazon.com/cdk/api/v2/python/aws_cdk.aws_lambda_nodejs/README.html#command-hooks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-lambda-nodejs bug This issue is a bug. effort/small Small work item – less than a day of effort p2
Projects
None yet
Development

No branches or pull requests

7 participants