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

fix(aws-kinesisfirehose-s3): Fix type of kinesisFirehoseProps prop. #1002

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ScottRobinson03
Copy link

@ScottRobinson03 ScottRobinson03 commented Aug 31, 2023

In V1.64.0 the type of the kinesisFirehoseProps prop was adjusted from kinesisfirehose.CfnDeliveryStreamProps to kinesisfirehose.CfnDeliveryStreamProps | any. I assume this was to prevent a type error around bucketArn and roleArn being required in kinesisfirehose.CfnDeliveryStreamProps, but not within the prop (since the construct handles setting these).

This was a lazy solution that meant developers haven't been able to benefit from the autocompletion of the type. The type has been corrected in this PR so that people can get autocomplete on the properties again.

The type admittedly isn't the prettiest, but does work and is the best way I could find to achieve the correct type.

I'm not sure why there's a | cdk.IResolvable; on the type originally, but I kept it here just in case.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: githubautobuild-for-cdk-v2
  • Commit ID: 0a8a637
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@ScottRobinson03
Copy link
Author

ScottRobinson03 commented Aug 31, 2023

Looks like there's something wrong with the build steps so the CodeBuild is failing... 🤔

@biffgaut
Copy link
Contributor

Thanks! Unfortunately, the issue here is that Solutions Constructs uses JSII to publish the libraries for Java and Python projects (the same library used by CDK to support multiple clients). The benefit of this is being able to serve more clients, but the drawback is that this imposes limitations in how we manipulate types in Typescript. We want to allow users to specify any subset of properties on incoming props without having to fill in all required properties (which we will supply as defaults), hence our use of any. Ideally, we'd use Partial<> for this but JSII can't translate this to other languages.

In this case, the build is working fine - but our build step is jsii rather than tsc. The first error that occurs in the log is:

@aws-solutions-constructs/aws-kinesisfirehose-s3: lib/index.ts:52:12 - error JSII1001: Non-primitive types without a symbol cannot be processed.

Which indicates to me that JSII cannot not process the omit utility type. That's disappointing, as we had not considered it previously and it was an intriguing possibility. If you check out the jsii repo, you can see that people have raised issues around Partial<>, and one of the team members explains the issue to some extent here.

@ScottRobinson03
Copy link
Author

ScottRobinson03 commented Sep 2, 2023

@biffgaut

Thanks! Unfortunately, the issue here is that Solutions Constructs uses JSII to publish the libraries for Java and Python projects (the same library used by CDK to support multiple clients). The benefit of this is being able to serve more clients, but the drawback is that this imposes limitations in how we manipulate types in Typescript. We want to allow users to specify any subset of properties on incoming props without having to fill in all required properties (which we will supply as defaults), hence our use of any. Ideally, we'd use Partial<> for this but JSII can't translate this to other languages.

In this case, the build is working fine - but our build step is jsii rather than tsc. The first error that occurs in the log is:

@aws-solutions-constructs/aws-kinesisfirehose-s3: lib/index.ts:52:12 - error JSII1001: Non-primitive types without a symbol cannot be processed.

Which indicates to me that JSII cannot not process the omit utility type. That's disappointing, as we had not considered it previously and it was an intriguing possibility. If you check out the jsii repo, you can see that people have raised issues around Partial<>, and one of the team members explains the issue to some extent here.

Hmm, interesting, I see. Thanks for the response.

After doing a little bit of research I came across aws/jsii#1707, which suggests using jsii-struct-builder to add support for utility types such as Omit. Is this something that has been looked into before?

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