Skip to content
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

Add support for Omit and Partial types to aid in extending Props interfaces #4468

Open
2 tasks
sam-goodwin opened this issue Mar 27, 2024 · 5 comments
Open
2 tasks
Labels
effort/medium Medium work item – a couple days of effort feature-request A feature should be added or improved.

Comments

@sam-goodwin
Copy link

Describe the feature

I'd like to be able to use Omit and Partial to create a sub-set of a Construct class I am sub-classing.

Use Case

I regularly extend L2 Constructs with opinionated (and more importantly, narrower) interfaces. For example, a L3 may require the underlying DB be Postgres and doesn't allow the engine to be set. But, I still want to expose the rest of rds.ServerlessClusterProps as optional props to be configured. Doing this today requires redundant copy/pasting of the props interfaces into the L3 Constructs which is wasteful and annoying in cases where the interfaces have deep hierarchies.

This should be possible:

export interface MyServiceProps extends Omit<rds.ServerlessClusterProps, "engine"> {}

// or also make all the props optional
export interface MyServiceProps extends Partial<Omit<rds.ServerlessClusterProps, "engine">> {}

Proposed Solution

JSII can leverage the type checker to resolve the properties on the omitted type and emit a new class that is a subset.

Other Information

No response

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

CDK version used

2.126.0

Environment details (OS name and version, etc.)

MacOS 14.1

@sam-goodwin sam-goodwin added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Mar 27, 2024
@mrgrain mrgrain added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed needs-triage This issue or PR still needs to be triaged. labels Apr 9, 2024
@mrgrain
Copy link
Contributor

mrgrain commented Apr 9, 2024

Thanks for the feature request.

JSII can leverage the type checker to resolve the properties on the omitted type and emit a new class that is a subset.

This is correct, jsii could do this. However the issue is more complicated for jsii languages that use a stronger type system.

What does it mean in Java if MyServiceProps partially extends rds.ServerlessClusterProps?
In my view this is code that simply cannot be represented in Java. MyServiceProps will also not be usable where rds.ServerlessClusterProps is expected - even if MyServiceProps were to only omit optional props.
What would your expectations be how this problem is solved?

I'm not convinced the added ambiguity is worth the added convenience.


In any case, today you can use third party tools like mrgrain/jsii-struct-builder to achieve a similar result.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Apr 10, 2024
@sam-goodwin
Copy link
Author

MyServiceProps will also not be usable where rds.ServerlessClusterProps is expected

This is not the desired behavior. Omit and Partial would create new types that don't extend the types they are created from.

export interface MyServiceProps extends Omit<rds.ServerlessClusterProps, "prop"> {}

It is common in Constructs to reduce or simplify the interface of the Constructs it wraps. Being able to create those narrow types without duplication is the feature request.

@mrgrain
Copy link
Contributor

mrgrain commented Apr 12, 2024

This is not the desired behavior. Omit and Partial would create new types that don't extend the types they are created from.

Acknowledged. That isn't your desired behavior. I'm posing the question if it would be confusing for some users.

I think interface MyServiceProps extends Omit<rds.ServerlessClusterProps, "prop"> {} is not equivalent to interface MyServiceProps { /* copy stuff here */ }. In Java et all we will be forced to use the second approach. What do you think the TS behavior should be then? Keep the original or also transform to the latter>

It is common in Constructs to reduce or simplify the interface of the Constructs it wraps. Being able to create those narrow types without duplication is the feature request.

Sure, I'm very familiar with the problem. "without duplication" is the challenge here. I believe it might cause ambiguity and am wondering about your view on how to resolve the ambiguity or if it even is a problem.

@sam-goodwin
Copy link
Author

I'm not sure I understand the ambiguity you're referring to.

interface MyServiceProps extends Omit<rds.ServerlessClusterProps, "prop"> {} is not equivalent to interface MyServiceProps { /* copy stuff here */ }

Why not? It is identical. It copies the properties across including their references and documentation.

I believe it might cause ambiguity and am wondering about your view on how to resolve the ambiguity or if it even is a problem.

What is the ambiguity? I am proposing that Omit and Partial be used to create entirely new types that don't extend the types they're derived from. It should behave as if I copied the properties from the target interface.

MyServiceProps will also not be usable where rds.ServerlessClusterProps is expected - even if MyServiceProps were to only omit optional props.
What would your expectations be how this problem is solved?

I am not following. What problem? MyServiceProps does not extend rds.ServerlessClusterProps in TypeScript because it lacks required properties. That will also be true in any of the generated languages. I don't think there's any ambiguity?

In Java et all we will be forced to use the second approach.

Yeah, that is what should happen in Java and other languages - a new type should be generated with properties copied across. That's ok because they are generated from TypeScript. The scope of this feature request is for the TypeScript->JSII compiler.

The desired behavior is to create new types that behave as if they were written by hand. They do not extend the types they are derived from.

@tijmenr
Copy link

tijmenr commented Apr 17, 2024

To start off, I would really welcome support for this in JSII, as indeed it makes it a lot easier to implement "L3" construct libraries.

(For reference, I am trying to use JSII to provide a Python version of an internal Typescript L3 construct library, ran into issues with interfaces extending Omit<...> that caused JSII to fail, found a workaround that at least made JSII succeed in producing a Python version, but then found out when trying to use it that JSII had basically ignored the "omitted interfaces", so the Python version is not usable. Then I googled and found this issue).

Regarding ambiguity, as Sam mentioned, a new interface (e.g., MyServiceProps) derived from an existing interface (e.g., rds.ServerlessClusterProps) via Omit or Partial, does not have a child-parent inheritance relation with that existing interface, and hence there is no ambiguity, you cannot use one in place of the other, and so I also don't see an issue here.

However, I do see a possible issue when the existing interface changes (which happens often with new minor CDK versions, as new features are implemented).

Consider an interface CdkProps { param1: string; param2: boolean; }, and you extend it as interface MyProps extends Omit<CdkProps, 'param1'>. So, it is equivalent to interface MyProps { param2: boolean; }.

Now, CDK releases a new minor version with a feature update on the interface, and it has changed to interface CdkProps { param1: string; param2: boolean | number, param3: string; }. In TypeScript, your MyProps interface now behaves accordingly. However, when people use MyProps via the Python/Java library you generated via JSII and made available a few weeks ago, in combination with the newer CDK (assuming you don't want to pin the CDK dependency for your library to a specific version, as that would prevent your users benefiting from new features/fixes in CDK, and you also don't want to build and release new versions of your own library in the same cadence as CDK itself), what does MyProps look like? What values does it allow for param2, and does it contain param3 or not?

Wishful thinking: I'd like that for loosely typed languages such as Python, it would basically mimic the Omit/Partial as defined in TypeScript, and for statically typed languages such as Java it retains whatever happened to be the state at JSII generation time, because that's the only option (and if that leads to inconsistencies of your library with newer CDK versions, then you can fix it by releasing a new version).
Edit: On second thought, trying to stick to the dynamic nature of Typescript interfaces is maybe not a good idea even for loosely typed languages such as Python, because the JSIII representation is also used for e.g. generating API documentation (and how would you document such a changeable type?). In any case, I'd already be very happy if JSII would fix the structure at code generation time.

(By the way: I had a quick look at jsii-struct-builder by @mrgrain, but I'm not sure it's really a solution. The main drawback that I see is that basically you have to start maintaining interface types outside of the locations where they logically belong, and also that (compared to the simple extends Omit<...>), it seems to add quite some complexity for the library developer (so it would be nice if that complexity could be taken care of by JSII ;) )).

@mrgrain mrgrain added the effort/medium Medium work item – a couple days of effort label May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/medium Medium work item – a couple days of effort feature-request A feature should be added or improved.
Projects
None yet
Development

No branches or pull requests

3 participants