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

Referenced TypeAliases #505

Merged
merged 20 commits into from
Jan 8, 2020
Merged

Referenced TypeAliases #505

merged 20 commits into from
Jan 8, 2020

Conversation

WoH
Copy link
Collaborator

@WoH WoH commented Oct 3, 2019

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you written unit tests?
  • Have you written unit tests that cover the negative cases (i.e.: if bad data is submitted, does the library respond properly)?
  • This PR is associated with an existing issue?

Closing issues

Closes #204, closes #429, closes #470

If this is a new feature submission:

  • Has the issue had a maintainer respond to the issue and clarify that the feature is something that aligns with the goals and philosophy of the project?

Potential Problems With The Approach

Test plan

  • alias a string with validation
  • alias a number with validation
  • alias a date with validation
  • alias union/intersection
  • alias a union of a model and an alias with validation on the alias
  • alias nestedObjectLiteral with validation on a property within the nOL.
  • generic type alias

For each of these:

  • Definition: Description
  • Definition: Example
  • Definition: Default
  • Definition: schema is correct
  • Definition: Validations are present
  • Definition: Model is fine
  • Validation: Validates
  • Validates: invalidates

Update:

  • Specs to ensure we check heritage clauses for properties from TypeAlias|MappedType|ConditionalType
  • Specs to ensure we check TypeAlias|MappedType|ConditionalType as type parameter defaults

Update 2 (out of scope for now):

  • nested contexts
  • nested context specs
  • Records

This is why this will probably take me a long time and I'd be super grateful if someone could contribute parts of that.

@WoH WoH force-pushed the refalias-v2 branch 2 times, most recently from b347d52 to 05bb851 Compare October 8, 2019 11:29
@WoH WoH changed the base branch from master to 3.x October 8, 2019 15:56
@WoH
Copy link
Collaborator Author

WoH commented Oct 8, 2019

Changed target to 3.x 🎆

@WoH WoH force-pushed the refalias-v2 branch 3 times, most recently from ed25b41 to 2c5c7fb Compare October 10, 2019 19:15
@WoH WoH force-pushed the refalias-v2 branch 2 times, most recently from a379fa2 to 1e7e025 Compare October 13, 2019 12:38
@github-actions github-actions bot added the Stale label Nov 26, 2019
@dgreene1 dgreene1 removed the Stale label Nov 26, 2019
@github-actions github-actions bot added the Stale label Dec 27, 2019
@dgreene1 dgreene1 removed the Stale label Dec 27, 2019
Repository owner deleted a comment from github-actions bot Dec 27, 2019
Repository owner deleted a comment from github-actions bot Dec 27, 2019
@WoH
Copy link
Collaborator Author

WoH commented Jan 2, 2020

@dgreene1 I don't wanna bother you with this, but I'd like to rebase this with master and cut a 3.0-alpha1 so users can give feedback. Wdyt?

@dgreene1
Copy link
Collaborator

dgreene1 commented Jan 2, 2020

@WoH that’s fine with me as long as I don’t have to publish a new version— users just npm install the branch. Is that acceptable to you?

@WoH
Copy link
Collaborator Author

WoH commented Jan 3, 2020

as I don’t have to publish a new version

Well that's exactly what I had in mind though.
A tsoa@next release 3.0.0-alpha1 so current users don't get the update unless explicitly asking for it.

I unfortunately don't have npm access or I'd offer to do it myself.

@dgreene1
Copy link
Collaborator

dgreene1 commented Jan 3, 2020

Luke and I decided that it’s best to have only one person who can deploy the new packages so that we don’t get overlaps.

As for getting feedback, if users want to try out the v3 branch, they can do that explicitly.

I don’t see any disadvantage for having them do npm install lukeautry/tsoa#v3branch.

However, there are disadvantages of having a next branch that would eventually fall behind master once v3 becomes the norm.

I truly feel very bad/guilty about not being able to give this PR the review attention it deserves. My plan was to actually merge this PR into master once the lingering v2 issues are resolved. Those lingering issues (for me) are the middleware issue, the security middleware issue, and the file response issue. I feel like that covers the 3 major items that users have been asking about.

But maybe it makes more sense to keep this all simple and to just accept that if users want those three major features they would have to get it by upgrading to the new major version. I honestly don’t know what’s best.

But for now I do know that there are no disadvantages to asking interested users to install the specific branch.

@WoH
Copy link
Collaborator Author

WoH commented Jan 3, 2020

The problem with the installation from github is that versioning is completely void and I may accidentally break builds with commits, making it even more unstable and probably completely unusable in any real scenario.

Those lingering issues (for me) are the middleware issue, the security middleware issue, and the file response issue. I feel like that covers the 3 major items that users have been asking about.

I'm not sure what you mean by security middleware, but I can assure you (properly swagger-aware) middleware is complicated enough to justify bumping another major.

As for the others I'll gladly take a look, but I think at this point it'd be easier to backport 2 PRs (that don't even exist yet) to 2.x than port 20 to 3.x, so I'd urge you to consider that aswell.

@dgreene1
Copy link
Collaborator

dgreene1 commented Jan 3, 2020

So @WoH are you saying that it might be better to just create the v3 branch now?

I could be okay with that. It might be the only solution that doesn’t involve me slowing you down.

@dgreene1
Copy link
Collaborator

dgreene1 commented Jan 4, 2020

Just wanted to follow up @WoH. I think we can do this. I was just reading this documentation: https://medium.com/@kevinkreuzer/publishing-a-beta-or-alpha-version-to-npm-46035b630dd7

It doesn't look too hard. So tell me if I understand the process properly:

  1. You would change the target of this PR from master to be v3beta
  2. All subsequent v3 PRs would also be merged to v3beta
  3. Whenever one of these PRs are merged, I will release a new version, but to the beta tag. This would allow users to try out our new versions.

Of course, the alternative is to just say "screw it" and we just merge this into master and publish tsoa package 3.0.0 and then we'd get lots of feedback! (haha)

I'm looking for your advice. Thank you for your patience. If I was still using tsoa for my day job then this would be more on my mind. But I'm busy having to learn how .NET Core works. 🤕

@WoH
Copy link
Collaborator Author

WoH commented Jan 6, 2020

  1. This PR already targets a 3.x branch
  2. Yes
  3. Yes, exactly, that's what I was hoping for

The idea of the next tag is to have a way to install a version that is not yet released.
See https://medium.com/@mbostock/prereleases-and-npm-e778fc5e2420

E: Rebased

I think this should be good to go as a first alpha, my plan would be to work on extractEnum + TS 3.7 compat for a2

This fixes an issue when you'd have a type like
let x: ForwardGenericAlias<number, number>
where
type ForwardGenericAlias<_T, U> = GenericAlias<U>;
where
type GenericAlias<T> = T;

The name created for GenericAlias<U> would be created incorrectly,
because the context for GenericAlias<T> created would only contain
T -> number, //forward referenced by FGA to GA
making it impossible to resolve it to the proper name:
GenericAlias_number_
If we build the context for this instance of GenericAlias<T> after
naming, the context still contains
_T -> number, U -> number
and the name resloves properly.
@dgreene1
Copy link
Collaborator

dgreene1 commented Jan 6, 2020

@WoH just a heads up, I'm going to call the tag v3prerelease instead of next because I don't want anyone to get the idea that we will be maintaining this tag after it gets merged to master. To put it another way, tsoa won't have a nightly build like typescript does. I would be worried that someone would subscribe to next with the hopes of always being on the newest. When in reality, after we merge a few PRs, we'll probably merge it all into master and assign the latest tag. Therefore latest will be newer than the next tag (which is why v3prerelease is a more appropriate tag).

@WoH
Copy link
Collaborator Author

WoH commented Jan 6, 2020

Sounds good

@dgreene1 dgreene1 merged commit 995ab39 into lukeautry:3.x Jan 8, 2020
@dgreene1
Copy link
Collaborator

dgreene1 commented Jan 8, 2020

@WoH the new tag should be out there. You can see it here: https://www.npmjs.com/package/tsoa

image

Note: Yarn doesn't support loose semver so I couldn't make the version 3.0.0.beta0 like I wanted to. But I got the tags right. So the people who install latest will still get v2. For my own notes, this is the command I ran:

yarn publish --new-version 3.0.0 --tag v3beta

@WoH
Copy link
Collaborator Author

WoH commented Jan 9, 2020

Have you tried

yarn publish --new-version 3.0.0-beta0 --tag v3beta

Maybe the . instead of the - is the issue?

I've prepped a little bit of text if you want to create a beta release for the changelog on GH:

3.0.0-alpha0

Welcome to the first prerelease in the 3.0 alpha cycle.

New Features:

Support for type aliases

This release comes with proper support for type alias definitions.

They can range from simple scenarios

/**
 * A Word shall be a non-empty sting
 * @minLength 1
 */
type Word = string;

to more complex scenarios like unions and intersections of aliases

type IntersectionAlias = { value1: string; value2: string } & TypeAliasModel1;

// or
type OneOrTwo = TypeAliasModel1 | TypeAliasModel2;

or even generic type aliases:

type GenericAlias<T> = T | string;
type ForwardGenericAlias<T, U> = GenericAlias<U> | T;

Please note that this means that tsoa does not only generate the specification (OpenAPI v3 and Swagger2*), but will also validate the input against the types including the jsDoc annotations.

* There may be certain scenarios where we may not be able to generate Swagger 2 from your TypeScript, tsoa will log warnings to infor you about any issues we are aware of.

Support for mapped types

TypeScript 2.1 introduced mapped types, a powerful addition to the type system. In essence, mapped types allow you to create new types from existing ones by mapping over property types. Each property of the existing type is transformed according to a rule that you specify. The transformed properties then make up the new type.
- Marius Schulz, https://mariusschulz.com/blog/mapped-types-in-typescript

tsoa now works with the ts type checker to resolve mapped types. We will actively try to support all cases, however the test suite for now only covers the utility mapped types typescript ships with, like:

/**
 * Make all properties in T optional
 */
type Partial<T> = {
    [P in keyof T]?: T[P];
};

/**
 * Make all properties in T required
 */
type Required<T> = {
    [P in keyof T]-?: T[P];
};

/**
 * Make all properties in T readonly
 */
type Readonly<T> = {
    readonly [P in keyof T]: T[P];
};

/**
 * From T, pick a set of properties whose keys are in the union K
 */
type Pick<T, K extends keyof T> = {
    [P in K]: T[P];
};

One of the types currently not covered yet is the Record type. Please note that this is experimental and there may be cases we unknowingly do not cover yet. Please report any issues you find

Support for conditional types

As of version 2.8, TypeScript supports conditional types. The syntax is very close to the ternary operator and enables expression of 2 (or more) different types based on a condition. Please refer to the TypeScript Handbook for details.

type Diff<T, U> = T extends U ? never : T;  // Remove types from T that are assignable to U

tsoa now works with the ts type checker to resolve conditional types. We will actively try to support most cases, however the test suite for now only covers the utility types typescript ships with, like:

/**
 * Exclude from T those types that are assignable to U
 */
type Exclude<T, U> = T extends U ? never : T;

/**
 * Extract from T those types that are assignable to U
 */
type Extract<T, U> = T extends U ? T : never;

/**
 * Exclude null and undefined from T
 */
type NonNullable<T> = T extends null | undefined ? never : T;

Support for combinations and utility types

The combination of mapped and conditional types allow for powerful utility types like the Omit type.

/**
 * Construct a type with the properties of T except for those in type K.
 */
type Omit<T, K extends keyof any> = Pick<T, Exclude<keyof T, K>>;

Again, experimental, there may be issues, please report.

Breaking changes

In order to support type aliases and avoid name clashes, the names for the generated component schemas / definitions may have changed (generic interfaces are affected mostly).
If you rely on the component names generated from tsoa, this is a breaking change.

Because tsoa supported some type aliases in the past and now generated definitions differently, this may break your code.
If you relied on tsoa not supporting type aliases properly to avoid issues, this may break your code.
Proceed with caution and report issues.

I (@WoH, on behalf of the maintainers) am are excited for your feedback!

ETA?

Feel free to follow the progress of the 3.0.0 release here.

Installation

yarn add tsoa@v3beta

@dgreene1
Copy link
Collaborator

Thanks for these release notes @WoH. They were super duper helpful!!!! :)

@Eywek Eywek mentioned this pull request Apr 8, 2020
4 tasks
@WoH WoH added the ba label Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants