Skip to content

Conversation

@gasolima
Copy link
Contributor

Refactor to put the common types in a separate file, and use proper generics instead of unions

@gasolima gasolima marked this pull request as ready for review November 17, 2025 10:48
@github-actions
Copy link
Contributor

@aws-cdk/aws-service-spec: No model change detected

@rix0rrr rix0rrr disabled auto-merge November 17, 2025 10:53
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provisionally approved, I'd like you to add some sugar to make the usage sites nicer (and potentially the diff smaller, if you so care)

@@ -0,0 +1,73 @@
import { Entity, Reference } from '@cdklabs/tskb';

export type PropertyType<T extends Entity> =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a line of documentation saying what T is for.

Also add a type alias in the original location so the consumer sites look nicer:

export type ResourcePropertyType = PropertyType<TypeDefinition>;

(Alternatively, make PropertyType the alias and give the parameterized type a different name, like GenericPropertyType<T>, to minimize the diff)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a line of documentation saying what T is for.

Forget to do this, but hopefully will do it in later PRs :D

@gasolima gasolima force-pushed the refactor-common-types branch 2 times, most recently from f13471c to 9a9a7e0 Compare November 17, 2025 12:51
@gasolima gasolima force-pushed the refactor-common-types branch from 9a9a7e0 to 57104e6 Compare November 17, 2025 12:52
@gasolima gasolima added this pull request to the merge queue Nov 17, 2025
Merged via the queue into main with commit c94c831 Nov 17, 2025
10 checks passed
@gasolima gasolima deleted the refactor-common-types branch November 17, 2025 13:22
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.

2 participants