Improve Generics Naming

[WoH]

The current naming for supported generic types (aka interfaces) is not very helpful.
Tsoa does some parsing around the model and the generics like MyModel<Item[]> becomes MyModelItemArray in the schema. However, this use case is very narrowly defined and might lead to more name clashes down the line. Passing literals like `MyModel<'id'> is entirely unsupported only because there is no case to get the name.

Any effort to fix #338 would also have to deal with this limitation.

Sorting

• I'm submitting a ...

  • bug report
  • feature request
  • support request
  • discussion

• I confirm that I

  • used the search to make sure that a similar issue hasn't already been submitted

Expected Behavior

See possible solution

Current Behavior

One example: MyModel<T | U> becomes MyModelobject and therefore overrides any other MyModel declarations with an object or a union.

Possible Solution

We do a lot of mostly unnecessary work when we could just use the name TypeScript gives the References (typeReferenceWithGenerics.getText()) and replace chars not allowed by RFC3986.

I.e. MyType<T, A | B> should be something like MyType_T.A|B_.

This is something that can be done in a few lines of code instead just blowing up the name finder for the type resolver.

Exemplary steps could be:

• Replace <> with __
• Replace , with .
• Replace [] with Array
• Replace & with And
• Strip escaped " or '

The amount of escapes here is properly specified and therefore, I'd argue, pretty reasonable to implement.

https://tools.ietf.org/html/rfc3986

Another option would be to %-encode (due to easy implementation), but it would hurt readability.

Exemplary types can be en/decoded here: https://www.url-encode-decode.com/

Context (Environment)

Version of the library: any recent version
Version of NodeJS: irrelevant

• Confirm you were using yarn not npm: [x]

Breaking change?

Depends. Yes if someone depends on the naming tsoa uses for models. However, this does fix a lot of reproducible problems with schema creation. Maybe it makes sense to ship this improvement in 3.0, but it would require a timely major version bump since the current procedure should impede any effort of confidently releasing improvements for generic types.

[dgreene1]

This sounds like a great idea. And since we only recently fixed the generic names, then I think it would be okay to release this as a minor or patch version if the existing unit tests pass. It sounds like this would simplify the code quite a bit. If so, then it sounds like a great idea. Thank you @WoH! :)

[WoH]

The unit tests do rely on specific naming, unfortunately.
With a new patch in place (%-encoding for ease of implementation) I'm at

• 744 passing
• 65 failing

Every time we check $ref for Promises, Arrays, Generics, the new name would be different, that's why.

It indeed removes a considerable amount of code inside the resolver.

I stumbled on a smaller potential issue I'll talk about later, but this is just about getting a feel for the preferred naming conventions we should follow.

[dgreene1]

@WoH, how do you feel this proposal would work with code generation?

I know that it’s out of tsoa’s scope to worry about someone trying to generate code from the swagger, but we have gotten complaints before (which is why that “hard assert” code is in there).

So if you get a chance to try out how pipe and period work in the names when swagger editor generates C# code (for instance), that would be helpful. I will also do some experiments too.

Anyway, we would be fine as long as this new naming pattern doesn’t hinder devs who like to create service libraries from swagger docs.

[WoH]

I've tried very hard to avoid potential issues considering the specs (json schema, uri, openapi & swagger) but I think if we can avoid issues with the swagger codegen I'd like to do that. Will report back.

[WoH]

which is why that “hard assert” code is in there

I assumed from the comments that this was supposed to ensure we follow the RFC3986 using percent-encoded URIs (which encodeURIComponent now does)

So if you get a chance to try out how pipe and period work in the names when swagger editor generates C# code (for instance), that would be helpful. I will also do some experiments too.

Do you have a link to that issue by any chance? I assume that specific codegen does not translate legal swagger/openapi3 names into valid c# names?

[WoH]

I tried the OpenAPI 3 generators from here since the swagger ones are all over the place for OpenAPI 3 specs.
Seems like they escape all invalid class/file names, which might lead to name clashes down the road, but are overall valid. (Not sure if they detect and rename though).

[dgreene1]

I know I’m very late to be asking for an alternative approach, but maybe this would be an easy change.

So it’s a small tweak on your recommendation (copies here for convenience):

Exemplary steps could be:

Replace <> with __
Replace , with .
Replace [] with Array
Replace & with And
Strip escaped " or '

I like all of that except the generic substitutions since I find the underscore to be less than readable. So I’d like to recommend an approach that has two benefits:

• It replaces all punctuation so it will have zero problems with code generators.
• I (subjectively) consider this approach more readable because it uses words instead of punctuation

Replace <> with Of
Replace , with AndOf
Replace [] with Array
Replace & with And
Replace | with Or
Strip escaped " or '

And to show how that would work:
MyGeneric<User, School> would become “MyGenericOfUserAndOfSchool”

Another example, would be Omit (which I know that we don’t support now, but there is a bounty out there for it). So Omit<User, “firstName” | “id”> would become “OmitOfUserAndOfFirstNameOrId”.

[WoH]

I thought about this when I made the proposal, but this would clash more easily.
I.e.:
GenericRequest<Generic<T, U>> would become GenericRequestOfGenericTAndOfU
GenericRequest<Generic<T>, U>would become GenericRequestOfGenericTAndOfU

Which means we need to indicate the closing > and I have not found a better solution for that.

[dgreene1]

I have three followup questions:

• (Question A) Oh okay, so the ending > become __ too? So (question A) if I’m understanding correctly, GenericRequest<Generic<T>, U> will become “GenericRequest__Generic__T__.U__” right?
• (Question B) I still find the period to be a strange thing to read, so what if we replaced comma with “With” so that GenericRequest<Generic<T>, U> became “GenericRequest__Generic__T__WithU__”
• (Question C) Is the ultimate goal of these PRs to get to conditional types? Because I feel like Omit, Exclude, Pick etc are going to be impossibly challenging to support.

[WoH]

One underscore for <, one for the >.
So GenericRequest<Generic, U> will become GenericRequest_Generic_T_.U_.
The main goal is to avoid name clashes, both with and generic interfaces (which already are an issue) and generic types (typeAliases).

[WoH]

Is the ultimate goal of these PRs to get to conditional types? Because I feel like Omit, Exclude, Pick etc are going to be impossibly challenging to support.

You said conditional, but you mentioned both mapped and conditional types.
I have an idea in mind for both, they are not that hard if you work with the Typechecker.
This means I might have to boil MappedTypes down to a interface-like thing which we already support and a conditional type down to a basic type or a declaration which we already support.

[dgreene1]

Great. So I’m aligned with your vision (I think). And the parts I don’t get, I trust and/or will help verify. So I recognize that what I’m about to say is a change of decisions, but maybe it really does make sense to change these PRs so they merge to a “v3Branch” branch?

That way when we’re read to go, we just merge master back into the v3Branch branch and we’d be able to release v3.

Because I want to make it easy on all of us, but I’m not sure exactly how to do that. Perhaps a long-lived branch is right idea? I’m totally open to ideas.

Oh and just so I’m clear, is the type alias dependent on the generic name change PR that we’re currently discussing? (My assumption is yes and that it would also be apart of the v3Branch branch)

[WoH]

I wanted to ask for a branch at some point, but it felt like I'd overstep my boundaries. That being said, I'd say it definitely makes sense if you want to cut a "big 3.0" release.
I recall a few issues we could put on a roadmap and get done on that branch without impacting 2.x users.

[dgreene1]

Yes and yes. A roadmap would be helpful I think. What is the best way to do that?

(I’d google it but I’m grocery shopping right now haha)

And if you’re okay with changing the PR merge destination to this branch then please do so. :)

[WoH]

GitHub supports this and you can add both PRs and issues to a milestone, maybe it's a good idea to make it visible to everyone so they can track our progress / find issues to help with.

I don't have the permissions, but you can add a milestone and attach the issues.

https://help.github.com/en/articles/about-milestones

[dgreene1]

Okay, I added a few of the PRs to the new board. Please let me know if I’m missing any across the multiple columns: https://github.com/lukeautry/tsoa/projects/1?add_cards_query=is%3Aopen#column-6640263

[WoH]

Maybe this and removing boolean as a valid setting for excessProps