cyclosproject / ng-openapi-gen

An OpenAPI 3.0 codegen for Angular
MIT License
397 stars 132 forks source link

Support algebraic combinators for object types #110

Closed hmil closed 3 years ago

hmil commented 4 years ago

The current implementation falls short when object types are combined with allOf, oneOf and anyOf.

It all boils down to this design choice: That allOf was implemented with interface inheritance.

In TypeScript, inheritance is a special case of an intersection type, with limitations. One major limitation being that union types cannot be composed with interface inheritance.

The intent of allOf is closer to the generic intersection type, which allows arbitrary combinations with union types.

Proposed solution

Let me know if you are open to this change and I can clean this up a bit.

hmil commented 4 years ago

Any interest in this bugfix?

luisfpg commented 4 years ago

The idea is interesting. At first sight, it seems to be more generic, and not to cause problems with existing codebase. I'm open to it, as long as there are no regressions. Unfortunately, I currently have no time to work on this project. When time permits I'll take a look.

luisfpg commented 4 years ago

I have finally found time to test the solution. It seems the way to go. Great job! I've tested generating using this fix on the https://github.com/cyclosproject/cyclos4-ui application, which heavily uses model inheritance, and everything worked with the new approach.

I see you have generated objects with allOf using the simple type. This is surely the easiest approach. However, the comments are lost and the files have everything in one line. The easiest would be to change the tsType function to generate comments and formatted code (maybe with an optional flag?).

Also, the model.ts and object.handlebars template should be updated, because the superclasses no longer have any use.

Finally, please, update the tests to make them pass again. And so, the PR can be merged.

hmil commented 4 years ago

I fixed the tests but I don't know how to go about the problem with comments. Also I don't think I will have enough time to tackle comments. Can you accept this even without a solution to the comments issue?

luisfpg commented 4 years ago

I'll leave this PR open for now and, when I have some time, I'll add the comments to the properties.

Lonli-Lokli commented 3 years ago

Maybe this PR can be merged?

luisfpg commented 3 years ago

@Lonli-Lokli AFAIR this PR had a broader scope than just the support for proper order of composition. It had the ultimate goal of replacing inheritance by Intersection types. So I think it needs some rework to be in shape for merging.

hmil commented 3 years ago

@Lonli-Lokli Feel free to take over from here.

The problem here is that currently interfaces are nicely formatted and have comments attached to them. With this change, the output is less legible and we lose comments.

All that is left to do is to figure out a way to make the output as nice as it was before and preserve the comments.

hmil commented 3 years ago

Nevermind I fixed it.

This is what the PPPlaceModel example looks like now:

import { PPEntityModel } from './pp-entity-model';
import { PPGpsLocationModel } from './pp-gps-location-model';
export type PPPlaceModel = PPEntityModel & (PPGpsLocationModel | {

/**
 * Street address
 */
'address'?: string;
}) & {
'description'?: string;
[key: string]: string;
};

I didn't bother with indentation, as it isn't a critical feature of this code generator. The important thing is to have documentation comments attached to the symbols, which is what this does.

Lonli-Lokli commented 3 years ago

Thats great :)

@luisfpg can you revisit it?

luisfpg commented 3 years ago

Yes, I've it on track, but I still need some time to check. I'll try to do it this weekend.

luisfpg commented 3 years ago

This was finally released in 0.18.0. Thanks a lot @hmil

hmil commented 3 years ago

Oh yeah! Thanks @luisfpg