feat: no more generated attribute types in CFN layer (L1) #1489

eladb · 2019-01-07T14:19:57Z

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 Tokens: be able to represent as numbers #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

BREAKING CHANGE: any CfnXxx resource attributes that represented a list of strings are now typed as string[]s (via #1144). Attributes that represent strings, are still typed as string (#712) and all other attribute types are represented as cdk.Token.

Pull Request Checklist

Testing
- Unit test added
- CLI change?: manually run integration tests and paste output as a PR comment
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
Title and Description
- Change type: title prefixed with fix, feat will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

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

tools/cfn2ts/lib/codegen.ts

rix0rrr · 2019-01-07T14:24:26Z

tools/cfn2ts/lib/codegen.ts

-      } 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();`);


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.

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.

RomainMuller

I reckon this is a breaking change - that should be mentioned on the PR description.

packages/@aws-cdk/alexa-ask/package.json

…tring[]"

RomainMuller · 2019-01-08T08:02:40Z

The Lazy concept described above... I'm afraid this won't translate super well in all languages (that's a little JSII-hostile). You can't really express Lazy.stringValue(() => someThing()) in Java for example, at least not in a way that'll achieve the desired outcome over JSII.

eladb · 2019-01-08T08:05:02Z

@RomainMuller I agree, but:

It's not worse than the current case (new Token(()=>...))
We should model it in a way that allows languages like Java to implement an interface instead of a lambda

mergify · 2019-09-23T15:08:00Z

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
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

eladb requested review from RomainMuller and rix0rrr January 7, 2019 14:19

eladb requested review from skinny85, SoManyHs and a team as code owners January 7, 2019 14:19

rix0rrr approved these changes Jan 7, 2019

View reviewed changes

RomainMuller approved these changes Jan 7, 2019

View reviewed changes

packages/@aws-cdk/alexa-ask/package.json Show resolved Hide resolved

Elad Ben-Israel added 5 commits January 7, 2019 16:35

Explicitly return void from emitResourceType

db92ac0

Stop generating "| Token" in L1 properties that accept "string" or "s…

62f35be

…tring[]"

Merge branch 'master' into benisrae/cfn2ts-remove-resource-attributes

61a3261

Stringify a couple of tokens

2d31a07

More stringifications

30eba0a

eladb merged commit 4d6d5ca into master Jan 8, 2019

eladb deleted the benisrae/cfn2ts-remove-resource-attributes branch January 8, 2019 08:13

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: no more generated attribute types in CFN layer (L1) #1489

feat: no more generated attribute types in CFN layer (L1) #1489

eladb commented Jan 7, 2019 •

edited

Loading

rix0rrr Jan 7, 2019

eladb Jan 7, 2019

RomainMuller left a comment

RomainMuller commented Jan 8, 2019

eladb commented Jan 8, 2019

mergify bot commented Sep 23, 2019

feat: no more generated attribute types in CFN layer (L1) #1489

feat: no more generated attribute types in CFN layer (L1) #1489

Conversation

eladb commented Jan 7, 2019 • edited Loading

Pull Request Checklist

rix0rrr Jan 7, 2019

Choose a reason for hiding this comment

eladb Jan 7, 2019

Choose a reason for hiding this comment

RomainMuller left a comment

Choose a reason for hiding this comment

RomainMuller commented Jan 8, 2019

eladb commented Jan 8, 2019

mergify bot commented Sep 23, 2019

eladb commented Jan 7, 2019 •

edited

Loading