fix(ecs): make Cluster and task definitions assignable to their interfaces under exactOptionalPropertyTypes#38097
fix(ecs): make Cluster and task definitions assignable to their interfaces under exactOptionalPropertyTypes#38097laazyj wants to merge 2 commits into
Conversation
…faces under exactOptionalPropertyTypes `ICluster` and `ITaskDefinition` declare several members optional (`?:`), but the concrete classes exposed them through getters typed `T | undefined`. Under TypeScript's `exactOptionalPropertyTypes`, a member typed `T | undefined` is not assignable to an optional `?: T`, so consumers who enable the flag get TS2420 errors and type-checking `aws-cdk-lib` fails for them outright. The library does not build with the flag, so the break is invisible to its own compile. Affected members: - `TaskDefinition.executionRole` (inherited by Ec2/External/Fargate task definitions), and - `Cluster.defaultCloudMapNamespace`, `Cluster.autoscalingGroup`, `Cluster.executeCommandConfiguration`. Convert each getter to a `public readonly x?: T` field. `executionRole` and `defaultCloudMapNamespace` are populated after construction (by `obtainExecutionRole` and `addDefaultCloudMapNamespace`), so they are set through a module-private helper that assigns through a `Writeable` cast; the live view is preserved. `autoscalingGroup` and `executeCommandConfiguration` are only set in the constructor, so they use a direct guarded `readonly` assignment. The fields stay `readonly`, so the jsii assembly is unchanged (the properties remain optional and immutable) and `yarn compat` passes with no new entry. The internal `ImportedCluster` is updated the same way so the module is fully clean. Add behavior tests (undefined by default, populated from props, and the live population via `obtainExecutionRole` / `addDefaultCloudMapNamespace`) and a package-local guard that type-checks the package's concrete classes against their interfaces under the flag, asserting the package stays clean. Refs aws#37996
|
Exemption Request: This is a type-only change — it converts existing read-only getters on |
aws-cdk-automation
left a comment
There was a problem hiding this comment.
The pull request linter fails with the following errors:
❌ Fixes must contain a change to an integration test file and the resulting snapshot.
If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.
✅ A exemption request has been requested. Please wait for a maintainer's review.
|
Correction to my exemption note above: to be precise about compatibility — |
|
Correction / resolution on the compat note: those |
Replace the hand-written assignability snippet with the shared `exactOptionalAssignabilityDiagnosticsForPackage` helper, which enumerates every exported, non-abstract class in the package and type-checks its assignability to each interface it implements (own and inherited) under the flag. Enumerating — rather than listing classes by hand — keeps the guard complete by construction: a newly-added offending class, or a new optional getter on an existing one, is caught automatically. Re-confirmed `Cluster` and the task definitions (executionRole / defaultCloudMapNamespace / autoscalingGroup / executeCommandConfiguration) are the only offenders in aws-ecs. Refs aws#37996
Issue # (if applicable)
Refs #37996
Reason for this change
IClusterandITaskDefinitiondeclare several members optional (?: T), but the concrete classes exposed them through getters typedT | undefined. Under TypeScript'sexactOptionalPropertyTypes, a member typedT | undefinedis not assignable to an optional?: T, so consumers who enable that flag hitTS2420errors and type-checkingaws-cdk-libfails for them outright, e.g.:aws-cdk-libdoes not build with the flag, so the break is invisible to the library's own compile and only surfaces downstream. This is one package in the sweep tracked by #37996.Affected members:
TaskDefinition.executionRole— inherited byEc2TaskDefinition,ExternalTaskDefinition, andFargateTaskDefinition;Cluster.defaultCloudMapNamespace,Cluster.autoscalingGroup, andCluster.executeCommandConfiguration.Description of changes
Convert each offending getter to a
public readonly x?: Tfield. Two population mechanisms are used, depending on where the value is written:executionRoleviaobtainExecutionRole,defaultCloudMapNamespaceviaaddDefaultCloudMapNamespace): populated through a small module-private helper that assigns via aWriteablecast. TypeScript forbids assigning areadonlyfield outside the constructor, so the cast is the minimal escape hatch; the live view is preserved (the property still reflects the post-construction population).autoscalingGroup,executeCommandConfiguration): a direct guardedreadonlyassignment — no cast needed.The fields stay
readonly, matching the original getters (no setter), so the jsii assembly is unchanged — every affected property remainsoptional: true, immutable: true, i.e.jsii-diffreports no change for this package. The internalImportedCluster(thefromClusterAttributespath) is updated the same way so the module is fully clean under the flag.A new per-package
aws-ecs/lib/private/writeable.tsholds the sharedWriteable<T>type (the package now has more than oneset*helper).Description of how you validated your changes
undefinedby default, populated from props, and — forexecutionRoleanddefaultCloudMapNamespace— correctly reflects the live post-construction population (obtainExecutionRole,addDefaultCloudMapNamespace), includingobtainExecutionRoleidempotency.aws-ecs/test/exact-optional-property-types.test.ts) that type-checks the package's concrete classes (Cluster,TaskDefinition,Ec2TaskDefinition,ExternalTaskDefinition,FargateTaskDefinition) against their interfaces withexactOptionalPropertyTypeson, asserting the package stays clean. Verified it goes red (TS2375) if the fix is reverted.jsii-diff(yarn compat) reports no change for this package — every affected property staysoptional: true, immutable: true. (Locally the command also surfaces a few unrelatedcloud_assembly_schemaremovals from the in-flight 2.258.0 release vs the last published baseline; those are not part of this change.) Lint clean; 192 tests pass.Note on the shared test harness and PR ordering
The guard uses a small shared helper,
exactOptionalAssignabilityDiagnosticsForPackage, added to@aws-cdk/cdk-build-tools. Several sweep PRs (one per package) each carry a byte-identical copy of this helper and import it, so they are ordering-agnostic and can land in any order. Whichever lands first establishes the helper; the others can be rebased to drop the duplicate before merge.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license