-
Notifications
You must be signed in to change notification settings - Fork 4k
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
L1s: properties are never typed anymore after being emitted as any
once
#32285
Labels
Comments
rix0rrr
added
bug
This issue is a bug.
needs-triage
This issue or PR still needs to be triaged.
labels
Nov 26, 2024
github-actions
bot
added
the
@aws-cdk/aws-cloudformation
Related to AWS CloudFormation
label
Nov 26, 2024
1 task
mrgrain
added
p1
and removed
needs-triage
This issue or PR still needs to be triaged.
labels
Nov 26, 2024
There are also some options to make the original type easier to use for at least some of our users.
|
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
The problem
Casing
Before we get into the issue, you first need to know the main difference between
any
-typed JSON data, and property typed data: casing!All properties in CloudFormation are PascalCase, but in most programming languages identifiers have different casing:
camelCasing
in TypeScript,snake_casing
in Python, etc. To make identifiers look natural, CDK translates the casing of identifiers when generating classes:CloudFormation:
This is all great, and we can do this if we know about the CloudFormation properties. However, sometimes properties in CloudFormation are typed as "arbitrary data" (colloquially called "JSON", even though it isn't really JSON because JSON is a string encoding 🤓). If the type in the schema is JSON, then we can't know the properties inside, and so we can't recase them.
So if we have:
The upshot is that if a property is typed as
any
, we just have to emit it as-is, and we have to leave the casing up to the customer.So they have to write:
Late typing additions
Everything works, but now comes a twist: a service team notices that they are typing a property as "JSON", and decide they can do better: they know what properties that field accepts, and they decide to add a type for it to the schema to help customers catch typing mistakes. So they change the schema:
This should be strictly better for customers: data that used to be valid is still valid, and data that used to be incorrect is now caught.
Except the first part of that (data that used to be valid is still valid) is true for CFN templates but not for CDK Code, and the reason is recasing.
Recall, a customer had written the following to correctly emit the
Minutes
field:But if we added the field typing, the underlying class definition would now look like this:
Adding types after the fact would break existing user code!
The current state
Which is why we don't do it. If a type has ever been typed as JSON, and therefore been emitted as
any
, it must forever be emitted asany
, and CDK customers cannot get the benefit of the added types!That's obviously not ideal, and regularly causes customer confusion.
The (proposed) solution
Even though we can't change the types of fields we've emitted in a backwards breaking way, there is another avenue available to us: adding new fields and deprecating old ones.
Once the additional types of
LongJob
become available to us, we could start emitting its properties like this:That way, we both don't break existing code, as well as give new users the opportunity to take advantage of the better types.
The mechanism to get here
The awscdk-service-spec has a
PreviousTypes
field that can be used for this. For properties whose types have gone through an evolution, it will have a history. The service spec data will look roughly like this:What the L1 generator currently does is always use the very oldest
PreviousType
to generate a field's property... but we could also be generating a field for every type.Those fields would obviously have to be mutually exclusive, and at rendering time we would pick the one that's supplied (if any), in code that looks somewhat like this:
There might be some subtle details to keep in mind while rolling this out. For example, a
required
field becomes optional, and we have to validate using runtime checks that exactly one (instead of at most one) of all the variants is supplied. Those issues should become self-evident once we start implementing this.The text was updated successfully, but these errors were encountered: