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

Discussion: Type Aliases should be aliases, not references #429

Closed
WoH opened this issue Aug 19, 2019 · 26 comments
Closed

Discussion: Type Aliases should be aliases, not references #429

WoH opened this issue Aug 19, 2019 · 26 comments

Comments

@WoH
Copy link
Collaborator

WoH commented Aug 19, 2019

Tl;Dr: Treat type aliases like symlinks, not like references (models/classes).

From the Typescript handbook on interfaces vs. type aliases:

One difference is that interfaces create a new name that is used everywhere. Type aliases don’t create a new name — for instance, error messages won’t use the alias name.

TSOA currently supports a very narrow set of type aliases: Interface aliasing and Intersections of Interfaces/Classes (where it collects the properties of all members).

Given #400, I would like to make aliases exactly what they are, convenient pointers to other types (unions, intersections, literals etc.) and treat them more or less like as a symlink.

However, this would break some swagger component/definition names (In my mind, the new representation is better, that's why I would prefer not to change it):

#/components/schemas/Namespace_InnerNamespace_TestModel
would become
#/components/schemas/Namespace.InnerNamespace.TestModel

(OpenAPI3, Swagger is similar)

Furthermore, there would no longer be a definition/component for the alias, the content would be inlined.

Thoughts?

@dgreene1
Copy link
Collaborator

This looks like a question for @lukeautry

@lukeautry
Copy link
Owner

lukeautry commented Aug 19, 2019

In the current framework, yeah, we don't really support type aliases, and the way that we do support them is as a pointer to an interface or a class, so I think your thoughts on this are reasonable.

I actually think full support for type aliases makes sense though, i.e. discriminated unions. It's possible that this has been implemented, but to my knowledge it has not.

If we have, say:

type OperationResult = IOperationSuccess | IOperationError;

interface IOperationSuccess {
  success: true;
  value: number;
}

interface IOperationError {
  success: false;
  message: string;
}

...the end user is probably thinking about OperationResult as a named data structure, not a simple pass through. But, in terms of what actually gets generated, the above would be a union type in Swagger, and in the routes would be handled similarly. The only place where one might expect to see OperationResult is in error messages, e.g. if we fail to resolve the type alias for some reason.

@WoH
Copy link
Collaborator Author

WoH commented Aug 19, 2019

Somewhat related: #204.
We could get rid of the issue by treating the alias like a pointer to a union, which can be properly resolved on #400
For ppl that prefer to look at it: WoH@d5abad9

Note that some of the fixed tests refer to new properties on the TestModel and therefore would not be considered a breaking change in any case.

@WoH
Copy link
Collaborator Author

WoH commented Aug 19, 2019

The only place where one might expect to see OperationResult is in error messages, e.g. if we fail to resolve the type alias for some reason.

If you're talking about ValidationErrors, here is what TS does:
(from the handbook):

In the code below, hovering over interfaced in an editor will show that it returns an Interface, but will show that aliased returns object literal type.

type Alias = { num: number }
interface Interface {
    num: number;
}
declare function aliased(arg: Alias): Alias;
declare function interfaced(arg: Interface): Interface;

@lukeautry
Copy link
Owner

I think it probably makes sense to align with what TypeScript does, then.

@HoldYourWaffle
Copy link
Contributor

I almost completely agree with treating aliases as "just symlinks", but there are two reasons why I'd want to keep my aliases in the output: documentation and readability.

I like to have aliases like type Word = string to better clarify the meaning of primitives, and in some cases these type aliases have additional validation constraints attached to them (for example, a Word must at least have a length of 1). I also have some more complex types that I gave an alias to improve code readability & decrease redundancy (probably the most common use for type aliases).

I'm not sure if this is really a "thing" in Swagger/OpenAPI since I don't use it myself (#360 (comment)). For me it would really help with code readability to be able to use the "unexpanded" type aliases in the router output.

I'm not saying we should keep things as they are or that we should support every use-case imaginable, we should definitely improve support for type aliases as much as possible. I just wanted to give my two cents on why one would want to keep the original alias names, in the hope that it will be taken into consideration.

I don't know the codebase well enough (yet) to know if this would reasonably be possible, but my first instinct says that it might be possible to store some kind of aliasName property on resolved alias types, so it can still be retrieved if needed. This might be total garbage however, and please feel free to tell me so if it is.

@WoH
Copy link
Collaborator Author

WoH commented Aug 24, 2019

I like to have aliases like type Word = string to better clarify the meaning of primitives, and in some cases these type aliases have additional validation constraints attached to them (for example, a Word must at least have a length of 1).

How would that validation look like. Are we talking about tsoa related validations here?
Please note that you can still do that just like before, I am only talking about how we internally represent that construct's metadata. Anywhere you'd say word: Word, we would internally transform that to: word: string.

I also have some more complex types that I gave an alias to improve code readability & decrease redundancy (probably the most common use for type aliases).

You can still do that. There just won't be a name for it. If there are errors you won't find the type name anywhere in there, that's more like what TS does, as I outlined here: #429 (comment)

I'd say it's reasonable for the error message to say 'hey, this is not a string' over 'hey, this is not a Word' given that's exactly what TypeScript would do. If you are using type aliases to create names you are "abusing" tsoa to do something TypeScript would not.

@HoldYourWaffle
Copy link
Contributor

(Sorry for my late response, English is not my first language so it took quite a while to properly formulate my thoughts on this, I know it looks long but please bear with me [TL;DR included])

How would that validation look like. Are we talking about tsoa related validations here?

No I'm using ajv with json-schemas (automatically generated by ts-json-schema-generator, check out this issue if you're interested). Sorry, I should've mentioned that immediately.

You can still do that. There just won't be a name for it.

This is exactly my concern. To properly explain why I'll have to give some additional information, so please bear with me. (TL;DR at the bottom)

One of the issues present in the "auto-generated json-schema community" (see the issue linked above) is that in some implementations type aliases are "lost" (as far as I can tell in the same way you're proposing here). This is a problem for '3' reasons:

  1. Type aliases can have additional information attached to them specific to that particular alias (the minimum length constraint in the Word example). This is reason number '0' because although functionality-wise it's not an issue (the constraints are just inlined into every reference), it's necessary information to understand the other two.
  2. This 'inlining' massively bloats and complicates the generated schema since there's no reusal of common "types". This means that every reference to Word will be generated as a string with a minLength constraint (adding another ~3 lines per reference). This doesn't sound that bad, but if you have more constraints and multiple aliases this gets really messy fast, leading to the next reason:
  3. It makes it basically impossible to read and verify the generated schemas as a human. I personally don't feel comfortable trusting an automatic generator (which has bugs) to handle a big part of my security on its own without any kind of human verification. Reading $ref: "Word" is a lot easier than checking if all the constraints match for every single reference.

So it makes sense to keep the aliases for json-schemas, but that's of course not what we're discussing here. The first reason (and the reason why I told this 'backstory') is that the "human-readable" issue applies to TSOA too. Of course on a much smaller scale (there's no "constraint-bloat"), but it's still very helpful to read generated routes with the alias names because it provides a form of self-documentation on the required constraints.
If aliases are lost this documentation will have to somehow be (manually) provided somewhere else which is inconvenient for both writers users of the API. TSOA might not be the one to handle it, but the client still has to comply with these constraints.
I'm using the router generation feature for both server and client so this self-documentation is probably more "directly noticeable" for me, but I'd think the same principle applies to swagger specs (please correct me if I'm wrong).

The second (and way more important) reason is that I want to link my routes to one of the auto-generated json-schemas (which will then verifiy my constraints). I can't do this if I don't know what alias (aka set of constraints) was used to define the route. As far as I know there's no workaround for this other than not using type aliases, which unfortunately isn't possible in a lot of cases.
Please keep in mind that Word is the simplest example I could think of, most of my type aliases are a lot more complicated (constraint-wise).

TL;DR: self-documentation and support for more complex validation requirements, in my case by linking routes to json-schemas which have additional constraints attached to them

I am only talking about how we internally represent that construct's metadata. Anywhere you'd say word: Word, we would internally transform that to: word: string.

The internal representation is of course completely up to TSOA, and your proposal definitely sounds like a welcome improvement to me.

I'd say it's reasonable for the error message to say 'hey, this is not a string' over 'hey, this is not a Word' given that's exactly what TypeScript would do.

I'm not sure what kind of error message you mean, but if you mean the default ValidatorService (which does exactly that, check if it's a string) I definitely agree.
However, if you're using a custom validator which has additional constraints attached to them like me, this simply isn't sufficient anymore (in my case AJV would throw an error telling me I don't match the required schema).

If you are using type aliases to create names you are "abusing" tsoa to do something TypeScript would not.

I'm not sure what you mean by 'abuse' here, as far as I know it's commonplace to use type aliases for documentation purposes. Again I'm not asking you to support every possible use-case possible, I'm just asking you to carefully consider this. If it's not reasonably possible to keep track of aliases (which would actually surprise me) and it allows for much greater improvements we should of course go in that direction (even if it means I won't be able to use TSOA anymore).

I also thought some more about a possible solution. Wouldn't an alias?: string property on Tsoa.Type be enough to track this? I'm pretty sure I did something similar when I implemented #353 to track the origin of a parameter.

@WoH
Copy link
Collaborator Author

WoH commented Aug 25, 2019

I'm not sure what kind of error message you mean, but if you mean the default ValidatorService (which does exactly that, check if it's a string) I definitely agree.

Yes, that's what I was referring to.

One of the issues present in the "auto-generated json-schema community" (see the issue linked above) is that in some implementations type aliases are "lost" (as far as I can tell in the same way you're proposing here).

That's pretty much what I'm proposing.

I'm not sure what you mean by 'abuse' here, as far as I know it's commonplace to use type aliases for documentation purposes.

Yes, but you should not rely on type aliases to create a name for you. Which is what you are doing when you are expecting it to $ref (and therefore $ref to a name that TS would not have created).
Passing the JSDoc and validators along should definitely happen.

  1. / 1. / 2.

I see your concerns with this idea and I will check if there is a way to mitigate some of the issues you presented. Unfortunately, type Word = string was never properly supported in tsoa at any point (afaik) so that's not even a breaking change and right now, I'm not clear how I could $ref the type of a property to a definition that just aliases a base type in Swagger or OpenAPI.
I assume this is the reason why the json schema generators have to inline aswell.
If there is a way I'd be happy to hear in otherwise I will consult my nearest specification.

The internal representation is of course completely up to TSOA, and your proposal definitely sounds like a welcome improvement to me.

I consider the content of the routes file internal though, I think you don't from what I am reading.

@dgreene1
Copy link
Collaborator

This 'inlining' massively bloats and complicates the generated schema since there's no reusal of common "types". This means that every reference to Word will be generated as a string with a minLength constraint (adding another ~3 lines per reference).

If a user of tsoa is concerned with bloat then they don’t have to use type aliases.

@HoldYourWaffle
Copy link
Contributor

Yes, but you should not rely on type aliases to create a name for you. Which is what you are doing when you are expecting it to $ref (and therefore $ref to a name that TS would not have created).
Passing the JSDoc and validators along should definitely happen.

I'm not sure why it matters how the typescript compiler exactly handles type aliases. We shouldn't try to make stuff do things that they obviously weren't designed for, but saving an aliased name doesn't sound too far fetched to me. (This is all assuming it's reasonably possible to support this "feature")

Unfortunately, type Word = string was never properly supported in tsoa at any point (afaik) so that's not even a breaking change

As far as I remember it worked just fine for me. It's been months since I've touched that project though (I'm working on TSOA right now as "preparation" and to reduce the forks I'm using in production) so I might remember wrong.

I'm not clear how I could $ref the type of a property to a definition that just aliases a base type in Swagger or OpenAPI.

I have absolutely no idea how the Swagger/OpenAPI format works so I can't help you with that.

I assume this is the reason why the json schema generators have to inline aswell.

There isn't a reason why they have to, it was just an implementation detail (one could say an unfortunate design choice). The implementation I linked is able to properly preserve aliases without issues (that I know of).

I consider the content of the routes file internal though, I think you don't from what I am reading.

I don't think so indeed, mostly because I'm using router generation for both server and client using custom templates, meaning that it's a direct public interface for me. In another way: I consider the information passed to the template (included or custom) to not be internal (because it's used an manipulated by users).

If a user of tsoa is concerned with bloat then they don’t have to use type aliases.

I can't tell if you mean TSOA is bloated or that concerned users shouldn't use type aliases 😅
If it's the latter: this is sadly not always possible (as far as I know).

@WoH
Copy link
Collaborator Author

WoH commented Aug 25, 2019

I'm not clear how I could $ref the type of a property to a definition that just aliases a base type in Swagger or OpenAPI.

To clarify: This should work technically fine for most cases. I'm not sure if we'd want to do that for very dynamic stuff, for example, mapped and conditional types or very highly generic types.

@simllll
Copy link
Contributor

simllll commented Sep 16, 2019

I'm not very familiar with all the technical details, but I guess I ran into something that could be related to this.

We have a lot of different object types (e.g. user, house, etc...). Every object has an object id, which is basically a string, but to distinguish between the different ids, and let typescript know what kind of ID i passed in, we are using following "type" for our ids:

export type TypeObjectId<T> = string & { type: T };

e.g.

class User {
_id: TypeObjectId<User>
}

class House {
_id: TypeObjectId<House>
houseOwners: TypeObjectID<User>[];
}

Alright, so far so good, but now the swagger file does not have the _id as a string, but as an "empty" object instead. It should be a string though... but preferable annotated / commented with the type of id that is required.

this is what I get:

TypeObjectIdUser:
            properties: {}
            type: object
            additionalProperties: false 

right now I use a regex to replace all this strings with "type: string". e.g. it becomes

TypeObjectIdUser:
            type: string
            additionalProperties: false 

It seems to work fine, but it's definitly a hack ;)

Would the "type alisa shoud be aliases" solve this issue? Or would you recommend some other appraoch to tackle this in a "unhackier" way?

Thanks,
I love this project!!

Regards
Simon

@WoH
Copy link
Collaborator Author

WoH commented Sep 16, 2019

Yes and no.
While your problem would be solved with the "pass-through" type aliases, they would also be handled just fine a proper implementation of "referenced" type aliases.
But the issue you're describing is caused by one of the reasons I'm trying to fix type aliases (either "pass-through" or "ref" style).

@WoH
Copy link
Collaborator Author

WoH commented Sep 18, 2019

I thought about this issue and while I think it's perfectly reasonable to "inline" type aliases, there are valid concerns and probably some missed opportunities I'd like to point out:

If we have, say:

type OperationResult = IOperationSuccess | IOperationError;

interface IOperationSuccess {
  success: true;
  value: number;
}

interface IOperationError {
  success: false;
  message: string;
}

...the end user is probably thinking about OperationResult as a named data structure, not a simple pass through. But, in terms of what actually gets generated, the above would be a union type in Swagger, and in the routes would be handled similarly. The only place where one might expect to see OperationResult is in error messages, e.g. if we fail to resolve the type alias for some reason.

Is it more intuitive to expect a named entity here? Yes, especially since OpenAPI supports this.

Also, @HoldYourWaffle raised an interesting point I hadn't had in mind initially:

I like to have aliases like type Word = string to better clarify the meaning of primitives, and in some cases these type aliases have additional validation constraints attached to them (for example, a Word must at least have a length of 1). I also have some more complex types that I gave an alias to improve code readability & decrease redundancy (probably the most common use for type aliases).

I think it would be nice to compose meaningful types like

 /**
   * This is a description of a special string property
   *
   * @example "asde"
   * @minLength 3
   * @maxLength 20
   * @pattern ^[a-zA-Z]+$
   */
type SpecialString: string;

to be reused throughout your application.

@dgreene1 @lukeautry @HoldYourWaffle What do you think?

This is a very powerful pattern that requires a lot more things happening internally though.
It impacts the type resolver, the spec generators, the route generation and the route templates + validation and should be specified through a lot of tests ensuring we cover all cases (Generic type aliases, descriptions, validations, examples etc).

This may be worth it, but I don't think I can spare sufficient time to add this on my own, so if this is something we should pursue I'd be very happy to get any support possible. Is there maybe a way to work on this on a special branch so everyone can contribute?

@dgreene1
Copy link
Collaborator

I'd be okay with that approach, but honestly I don't use type aliases enough to have a personal investment in this kind of feature.

This may be worth it, but I don't think I can spare sufficient time to add this on my own, so if this is something we should pursue I'd be very happy to get any support possible. Is there maybe a way to work on this on a special branch so everyone can contribute?

So in order for me to "get invested," I'd need to see some tsoa users write in to share why better alias support helps them get work accomplished. If the community really wants it then I would definitely jump in.

@WoH
Copy link
Collaborator Author

WoH commented Sep 19, 2019

I'd be okay with that approach, but honestly I don't use type aliases enough to have a personal investment in this kind of feature.

Glad to hear that. I think is still a relevant topic because nearly all of the "modern" TS convenience types are type aliases of mapped, conditional or some other types. So I'd really like to get this "right".

So in order for me to "get invested," I'd need to see some tsoa users write in to share why better alias support helps them get work accomplished. If the community really wants it then I would definitely jump in.

If I could get help with specs, code review, and docs from anyone that'd really really help me out.
But I agree, there should be interest from the community.

Edit: Issues that need a proper type alias impl: #469, #372, #204, #151 (mapped type needed)

@dgreene1
Copy link
Collaborator

Edit: Issues that need a proper type alias impl: #469, #372, #204, #151 (mapped type needed)

Yea, don't forget #210 which actually has a bounty prize on it.

If I could get help with specs, code review, and docs from anyone that'd really really help me out.

Yea, I think if we have a spec that verifies Omit and Exclude work as expected, then we'd be in a good spot.

I can help with the specs if/when the time comes.

@WoH
Copy link
Collaborator Author

WoH commented Sep 20, 2019

Yea, I think if we have a spec that verifies Omit and Exclude work as expected, then we'd be in a good spot.

Just to clarify: The bare alias implementation won't make those work, but they are a necessary precondition for them to work.

We still need a PR that properly parses these (MappedType)Nodes in the AST, but we can't even get to that point if the alias node already won't work.

So first we have to address type aliases to tsoa compatible types first (in a way that also works out well with future node types).

@HoldYourWaffle
Copy link
Contributor

So if I understand correctly, you want to keep the alias references to allow the 'meaningful types' you mentioned? I'm very tired so I'm sorry if I'm missing something obvious here...

@WoH
Copy link
Collaborator Author

WoH commented Sep 27, 2019

No, please read again. when you are not tired.

@HoldYourWaffle
Copy link
Contributor

After reading it again and scanning #485 I still don't think I understand. It looks to me like your saving the reference, just in a different format with extra handling.

@WoH
Copy link
Collaborator Author

WoH commented Sep 28, 2019

What are "alias references" in your mind and what do you mean by "saving the reference"?
The main difference between my previous approach is that instead of inlining all the type information, I now create a schema that can be linked to in swagger/openapi.

E: And I generate a seperate validation model for each type alias.

@WoH WoH mentioned this issue Oct 4, 2019
19 tasks
@WoH WoH added this to the 3.x milestone Oct 8, 2019
@github-actions
Copy link

github-actions bot commented Nov 8, 2019

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

@givethemheller
Copy link

@HoldYourWaffle @dgreene1 or @WoH

Can anyone TLDR this thing?
It relates to #372 which I've been getting at lately.

@WoH
Copy link
Collaborator Author

WoH commented Mar 9, 2020

TL;Dr: Type aliases are proper references to a OpenAPI component / Swagger definition and merged in the 3.0 branch which is in beta and will be released once all issues in the 3.0 backlog are addressed. Please refer to the Project tracker.

@WoH WoH closed this as completed Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants