aws-solutions-constructs
aws-solutions-constructs copied to clipboard
awslabs
Replace | any with Partial<T> for type checking across all constructs
Could Partial<cloudfront.DistributionProps> not be used instead?
Originally posted by @naseemkullah in https://github.com/awslabs/aws-solutions-constructs/issues/247#issuecomment-869812304
If we do it, it will be pretty massive - as we use this pattern EVERYWHERE, so it will affect virtually every construct (consistent behavior across constructs is one of our fundamental underlying tenets). But at first glance, it appears that it wouldn't break existing code nor screw up any unit or integration tests.
Alright, I do not see how it would break anything as it would just disallow passing in props that are not present in the type (while making them all optional which solves the use case of users just overriding a subset of required props). Once you are convinced please let me know if you'd like me to submit a PR.
Our next sprint meeting is Tuesday (delayed a day because of the US holiday). We'll discuss it as a team then - my guess is that we will take you up on the PR offer after that. :-)
We use a package called JSII to create the interfaces on top of Constructs that allow them to be used in Java, Python, and (eventually) .NET. We copied this pattern from the AWS CDK, which does the same thing. While Partial<> works as a | any replacement for an all Typescript/Javascript environment, it confuses JSII when trying to create the other libraries - this prevents us from using Partial<> at this time.
Details-
Partial creates a new interface that is a Mapped Type, which in this case is apparently not string indexed. The error when running JSII is error JSII1003: Only string-indexed map types are supported.
We also attempted to convert the mapped type to a different type using export interface PartialProps extends Partial<KinesisEventSourceProps> {};. This starts to show the complexities of Mapped Types, as we get the error error JSII3004: Illegal extends clause for an exported API: MappedType
We've entered an issue with the JSII team.
@biffgaut why was this closed? from your last message I was under the impression that after fixing the issue within JSII we would be able to use Patrial types, is that not the case?
Closing this issue is just a housekeeping thing. If JSII addresses the issue we will re-open this, as we really like the idea. But I'm guessing it's a big ask for JSII to address the issue. I'll reopen this for now, but if the JSII issue stays dormant for a long time or comes back with "sorry, can't do it" we'll reevaluate.