-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat: no more generated attribute types in CFN layer (L1) #1489
Conversation
Now that we can represent attributes as native strings and string lists, this covers the majority of resource attributes in CloudFormation. This change: * String attributes are represented as `string` (like before). * String list attribute are now represented as `string[]`. * Any other attribute types are represented as `cdk.Token`. Attributes that are represented as Tokens as of this change: * amazonmq has a bunch of `Integer` attributes (will be solved by #1455) * iot1click has a bunch of `Boolean` attributes * cloudformation has a JSON attribute * That's all. A few improvements to cfn2ts: * Auto-detect cfn2ts scope from package.json so it is more self-contained and doesn't rely on cdk-build-tools to run. * Added a "cfn2ts" npm script to all modules so it is now possible to regenerate all L1 via "lerna run cfn2ts". * Removed the premature optimization for avoiding code regeneration (it saved about 0.5ms). Fixes #1406
} else { | ||
this.code.line(`this.${at.propertyName} = new ${at.attributeType.typeName.className}(${at.constructorArguments});`); | ||
if (at.attributeType === 'string') { | ||
this.code.line(`this.${at.propertyName} = ${at.constructorArguments}.toString();`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if this shouldn't be the L2s responsibility? Shouldn't we leave everything as cdk.Token
here and have the L2 call the appropriate .toString()
or .toList()
?
We don't have to hide Tokens
completely at the L1, and we can't either because numbers cannot be represented in the type system, and booleans never will be. So why do a half-assed job? Better to abstain completely I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think encoding the type info in L1 is useful for L2 authors (and anyone who uses L1 directly). Granted, there are about 6 attributes in the whole surface area that are now untyped (I can add some docstrings about that), but all the rest are strongly-meta-typed, and align with our approach for modeling tokens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reckon this is a breaking change - that should be mentioned on the PR description.
The |
@RomainMuller I agree, but:
|
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
Now that we can represent attributes as native strings and string lists,
this covers the majority of resource attributes in CloudFormation.
This change:
string
(like before).string[]
.cdk.Token
.Attributes that are represented as Tokens as of this change:
Integer
attributes (will be solved by Tokens: be able to represent as numbers #1455)Boolean
attributesA few improvements to cfn2ts:
and doesn't rely on cdk-build-tools to run.
to regenerate all L1 via "lerna run cfn2ts".
(it saved about 0.5ms).
Fixes #1406
BREAKING CHANGE: any
CfnXxx
resource attributes that represented a list of strings are now typed asstring[]
s (via #1144). Attributes that represent strings, are still typed asstring
(#712) and all other attribute types are represented ascdk.Token
.Pull Request Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.