Internal refactor: "tagged union" allows us to remove almost all type assertions in TypeResolver #495

dgreene1 · 2019-10-03T05:29:47Z

This implements the tagged union solution recommended by @lukeautry as a solution for reducing the number of type assertions (i.e the as operator).

This should resolve the refactoring that was presented in #457 and that was designed to make tickets such as #452 easier to implement.

As recommended by Luke and presented by @HoldYourWaffle, there are no new features or bug fixes. It's just a refactor and therefore I tried to make as minimal changes as possible to the test code so that it was clear that no functionality should have changed with the refactor.

dgreene1 · 2019-10-03T05:32:28Z

@WoH I can not figure out why this refactor broke two of the integration tests. One of them is the "should call out any additionalProperties on Unions" test that you wrote, so would you be willing to give a second set of eyes to help me figure out what I did wrong?

Also, since this code touches a lot of the v3 work you're doing, please review the changes and let me know if/when you think that it should get into master (i.e. as part of v2 or as part of v3)

WoH · 2019-10-03T10:28:40Z

I'm doing something similar with the refTypes in the TypeAlias PR:
a91dae8

So generally, I like the approach. But I have strong feelings concerning the (new) names.

WoH

Initial review. I haven't checked it out locally yet, which I will do for round 2.
While I like the benefits of stricter types, I don't see a reason to change that much of the underlying program logic.

dgreene1 · 2019-10-03T12:34:28Z

README.MD

@@ -223,6 +223,16 @@ export interface MyModel {
 ```

 ### Overriding route template
+


It’s only related because I had to make changes that would break everyone who has a custom template. Those people using custom templates should probably be told that they’re at risk. If not, then we have to consider every data change to the templates as a breaking change. I don’t want to do that many major versions because people will miss out on cool features then.

WoH · 2019-10-03T10:30:07Z

src/metadataGeneration/initializer-value.ts

@@ -1,7 +1,7 @@
 import * as ts from 'typescript';
 import { Tsoa } from './tsoa';

-export const getInitializerValue = (initializer?: ts.Expression, type?: Tsoa.Type) => {


I would strongly prefer to keep the name as it is and only changing the underlying type declaration.

Oh yea, I changed the name in the first place so that I could see (via compiler error messages) where I needed to update the code. Essentially I was using the compiler as my pair-programming buddy.

I fully intended to change the name back.

But honestly, I have no intention of merging a PR that makes development on the project harder. After all, refactor is supposed to:

improve speed of development

it didn’t in this case because the new strictness caused some weirdness

improve code quality

well, two tests are broken that I can’t fix so I can’t say that quality was improved

And most importantly, there’s the subjective quality of the work. If you would rather not work on top of this PR, then we shouldn’t merge it.

Either way, the name Type stays the same.

WoH · 2019-10-03T10:30:22Z

src/metadataGeneration/metadataGenerator.ts

@@ -87,4 +87,4 @@ export class MetadataGenerator {
      .filter(generator => generator.IsValid())
      .map(generator => generator.Generate());
  }
-}


Seems like you avoided all checks to be able to push this.
You can still get the formatting to not be off by running yarn run prettier **/*.ts --write

WoH · 2019-10-03T10:34:15Z

src/metadataGeneration/typeResolver.ts

@@ -127,7 +130,7 @@ export class TypeResolver {
        additionalType = new TypeResolver(indexSignatureDeclaration.type as ts.TypeNode, this.current, this.parentNode, this.extractEnum, this.context).resolve();
      }

-      const objLiteral: Tsoa.ObjectLiteralType = {
+      const objLiteral: Tsoa.NestedObjectMetaType = {


Suggested change

const objLiteral: Tsoa.NestedObjectMetaType = {

const objLiteral: Tsoa.NestedObjectLiteralType = {

Please don't change the naming without reason. I agreed to add nested so new contributors can read this more easily. However, TS calls these ObjectLiterals, so everyone familiar with the TS terminology should be respected aswell.

dgreene1 · 2019-10-03T12:32:25Z

src/metadataGeneration/typeResolver.ts

    if (typeReference.typeArguments && typeReference.typeArguments.length > 0) {
      this.typeArgumentsToContext(typeReference, typeReference.typeName, this.context);
    }

-    referenceType = this.getReferenceType(typeReference.typeName as ts.EntityName, this.extractEnum, typeReference.typeArguments);


The only advantage it provides is that refObjects definitely don’t have an enums property and refEnums definitely don’t have a properties property.

In the current code, the interface has all properties as nullable.

But yes, I hear you. The ramifications of the tagged union approach were much more than I expected. So if you don’t see an advantage in the approach, then we can close this PR.

WoH · 2019-10-03T10:39:17Z

src/routeGeneration/routeGenerator.ts

+/**
+ * This will help us do exhaustive matching against only reference types
+ */
+export function isRefType(metaType: Tsoa.MetaType): metaType is Tsoa.ReferenceType {


should be util.
We may want to add more of these.

tests/fixtures/custom/custom-tsoa-template.ts.hbs

WoH · 2019-10-03T10:42:39Z

tests/unit/templating/templateHelpers.spec.ts

+/**
+ * This is a convenience type so you can check .properties on the items in the Record without having TypeScript throw a compiler error. That's because this Record can't have enums in it. If you want that, then just use the base interface
+ */
+interface ModelsButOnlyRefObjects extends TsoaRoute.Models {


Should be defined next to Models as RefObjectModels.

Oh I can do that, but I put it in the test since in reality the mapping/dictionary always has the potential for having both refEnums and refObjects. In other words, ModelsButOnlyRefObjects is a "test-only" type. Would you still like me to make this change now that I've clarified?

I'm okay with making your proposed change or not. I don't have strong feelings on the matter, I just wanted to clarify why it's in templateHelpers.spec.ts.

dgreene1 · 2019-10-03T12:32:25Z

src/metadataGeneration/typeResolver.ts

    if (typeReference.typeArguments && typeReference.typeArguments.length > 0) {
      this.typeArgumentsToContext(typeReference, typeReference.typeName, this.context);
    }

-    referenceType = this.getReferenceType(typeReference.typeName as ts.EntityName, this.extractEnum, typeReference.typeArguments);


The only advantage it provides is that refObjects definitely don’t have an enums property and refEnums definitely don’t have a properties property.

In the current code, the interface has all properties as nullable.

But yes, I hear you. The ramifications of the tagged union approach were much more than I expected. So if you don’t see an advantage in the approach, then we can close this PR.

dgreene1 · 2019-10-03T12:34:28Z

README.MD

@@ -223,6 +223,16 @@ export interface MyModel {
 ```

 ### Overriding route template
+


It’s only related because I had to make changes that would break everyone who has a custom template. Those people using custom templates should probably be told that they’re at risk. If not, then we have to consider every data change to the templates as a breaking change. I don’t want to do that many major versions because people will miss out on cool features then.

src/routeGeneration/templates/express.hbs

dgreene1 · 2019-10-03T12:41:22Z

src/metadataGeneration/tsoa.ts

    description?: string;
    dataType: RefTypeLiteral;
    refName: string;
-    properties?: Property[];
-    additionalProperties?: Type;
-    enums?: string[] | number[];


@WoH these three lines above are probably the only part of this PR that can clarify the possible value that this PR provides. Right now it looks like a ReferenceType can have all of these properties or none. Which means that the compiler can’t help us enforce that we give the right properties to the right type.

I certainly agree that this provides value ;)

Okay yea, so 50% (or more) of the value here has already been achieved in your other PR. That’s more reason to close this.

I disagree. The value does not only lie in fewer bugs/better checking of the existing code but a better DX for contributors. Let's not underestimate that

dgreene1 · 2019-10-03T12:43:37Z

Long story short, we don’t have to merge this PR at all. I wanted to try the challenge of doing the refactor that HoldYourWaffle proposed. But if this change is perceived as being “more harm than good” then it shouldn’t be merged.

Perhaps there are bits and pieces that can be preserved and used in future (less invasive) PRs.

I’m open to any and all ideas.

WoH · 2019-10-03T12:59:37Z

But if this change is perceived as being “more harm than good” then it shouldn’t be merged.

Let me be clear. It's very good but it certainly creates more work, especially the change of control flow in the type resolver.

If you can address:

the failing tests
formatting
type naming
moving one function to utilities

I think it's worth the effort. Which was my position with the PR that inspired this one aswell.

dgreene1 · 2019-10-03T13:30:11Z

@WoH which would you prefer:

A) you incorporate whatever parts of this PR into your work
B) you rebase after I fix this

I’m fine with either. I’m leaving pretty strong towards Option A since it would be more fair to you who has multiple open PRs right now.

In fact, I’m going to close this PR and we can always reopen it if need be.

WoH · 2019-10-03T13:39:27Z

I already got comfortable with rebasing and started developing a plan in my mind ;)

If you can address these issues I'm leaning very much towards B.

E: I removed some of the comments to reduce confusion, the ones still standing should be the only ones still relevant.

…templates at some point so these mistakes are caught in the typescript compiler

…ith it's other similar types.

WoH

This is really good now.
I actually think we should make sure we determine additionalProps on all RefModels.

Seems like you need to rebase though.

WoH · 2019-10-03T16:40:05Z

src/routeGeneration/tsoa-route.ts

+
+  export interface RefObjectModelSchema {
+    dataType: 'refObject';
+    properties: { [name: string]: PropertySchema };
    additionalProperties?: boolean | PropertySchema;


Suggested change

additionalProperties?: boolean | PropertySchema;

additionalProperties: boolean | PropertySchema;

I know this will cause TS to complain, but fixing the complaints might be worth the additional safety, what do you think?.

I understand where you’re coming from, but Swagger doesn’t require that field. So I personally feel like that makes debugging things harder.

WoH · 2019-10-03T16:45:27Z

src/utils/internalTypeGuards.ts

+/**
+ * This will help us do exhaustive matching against only reference types. For example, once you have narrowed the input, you don't then have to check the case where it's a `integer` because it never will be.
+ */
+export function isRefType(metaType: Tsoa.Type): metaType is Tsoa.ReferenceType {


Suggested change

export function isRefType(metaType: Tsoa.Type): metaType is Tsoa.ReferenceType {

export function isRefType(type: Tsoa.Type): type is Tsoa.ReferenceType {

WoH

LGTM

WoH · 2019-10-03T17:45:36Z

@dgreene1 This is the not the best place to ask, but can you open the 3.x branch so I can rebase my PRs? Luke hasn't responded to my request yet.

dgreene1 · 2019-10-03T17:50:29Z

Btw, @WoH I'd really like to take this further since the biggest DX problems (for me) are in TypeResolver. But when I tried to use exhaustiveness checking there, it uncovered that there are quite a few types that we don't specifically validate and instead we just "fallthrough" to this.validateModel.

What do you think?

New PR? Leave it alone?

Personally, I'd love to see function get upgraded to use more narrow types. Like once you know that dataType is "enum" then you know that it's .enums property will be there. So the members? parameter of validateEnum can become members (i.e. non-nullable)
So, instead of:

public validateEnum(name: string, value: any, fieldErrors: FieldErrors, members?: Array<string | number>, parent = ''): any {

public validateEnum(name: string, value: any, fieldErrors: FieldErrors, members: Array<string | number>, parent = ''): any {

That's just one example. See, this is why I don't like to refactor for refactoring sake... cause once I start I can't stop myself! haha

WoH · 2019-10-03T18:02:08Z

What you are pointing out is an issue that should be considered a bug discovered by refactoring.
I'd suggest submitting a PR that addresses these bugs.

Fyi, the check for .ref makes sure we only validate the model, so they don't fall through to validate model, they just don't get checked.

A simple refactoring PR that makes this explicit and creates a branch for each of the unvalidated types should be pretty minimal.

removed almost all type assertions. Only two tests are failing

f37073e

WoH reviewed Oct 3, 2019

View reviewed changes

dgreene1 commented Oct 3, 2019

View reviewed changes

WoH requested changes Oct 3, 2019

View reviewed changes

src/routeGeneration/templates/express.hbs Outdated Show resolved Hide resolved

dgreene1 commented Oct 3, 2019

View reviewed changes

dgreene1 closed this Oct 3, 2019

dgreene1 reopened this Oct 3, 2019

dgreene1 added 3 commits October 3, 2019 10:55

fixes the broken unit test. We really should refactor the handlebars …

457407c

…templates at some point so these mistakes are caught in the typescript compiler

change names back to what they were before

2a0f06c

re-run prettier

7008a17

dgreene1 changed the title ~~removed almost all type assertions. Only two tests are failing~~ Internal refactor: "tagged union" allows us to remove almost all type assertions in TypeResolver Oct 3, 2019

dgreene1 added 2 commits October 3, 2019 12:06

moved isRefType to a util module. Moved RefObjectModels out to live w…

65c902f

…ith it's other similar types.

Merge branch 'master' into tagged-union-refactor

0579946

WoH reviewed Oct 3, 2019

View reviewed changes

Merge branch 'master' into tagged-union-refactor

e6ed103

WoH approved these changes Oct 3, 2019

View reviewed changes

dgreene1 merged commit 235dc7a into lukeautry:master Oct 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Internal refactor: "tagged union" allows us to remove almost all type assertions in TypeResolver #495

Internal refactor: "tagged union" allows us to remove almost all type assertions in TypeResolver #495

dgreene1 commented Oct 3, 2019

dgreene1 commented Oct 3, 2019

WoH commented Oct 3, 2019

WoH left a comment •

edited

Loading

dgreene1 Oct 3, 2019

WoH Oct 3, 2019

dgreene1 Oct 3, 2019

WoH Oct 3, 2019

WoH Oct 3, 2019

WoH Oct 3, 2019

dgreene1 Oct 3, 2019

dgreene1 Oct 3, 2019

WoH Oct 3, 2019

WoH Oct 3, 2019

dgreene1 Oct 3, 2019

dgreene1 Oct 3, 2019

dgreene1 Oct 3, 2019

dgreene1 Oct 3, 2019

WoH Oct 3, 2019

dgreene1 Oct 3, 2019

WoH Oct 3, 2019

dgreene1 commented Oct 3, 2019 •

edited

Loading

WoH commented Oct 3, 2019 •

edited

Loading

dgreene1 commented Oct 3, 2019

WoH commented Oct 3, 2019 •

edited

Loading

WoH left a comment •

edited

Loading

WoH Oct 3, 2019

dgreene1 Oct 3, 2019

WoH Oct 3, 2019

WoH left a comment

WoH commented Oct 3, 2019 •

edited

Loading

dgreene1 commented Oct 3, 2019

WoH commented Oct 3, 2019

		@@ -223,6 +223,16 @@ export interface MyModel {
		```

		### Overriding route template

	const objLiteral: Tsoa.NestedObjectMetaType = {
	const objLiteral: Tsoa.NestedObjectLiteralType = {

	additionalProperties?: boolean \| PropertySchema;
	additionalProperties: boolean \| PropertySchema;

	export function isRefType(metaType: Tsoa.Type): metaType is Tsoa.ReferenceType {
	export function isRefType(type: Tsoa.Type): type is Tsoa.ReferenceType {

Internal refactor: "tagged union" allows us to remove almost all type assertions in TypeResolver #495

Internal refactor: "tagged union" allows us to remove almost all type assertions in TypeResolver #495

Conversation

dgreene1 commented Oct 3, 2019

dgreene1 commented Oct 3, 2019

WoH commented Oct 3, 2019

WoH left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgreene1 commented Oct 3, 2019 • edited Loading

WoH commented Oct 3, 2019 • edited Loading

dgreene1 commented Oct 3, 2019

WoH commented Oct 3, 2019 • edited Loading

WoH left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WoH left a comment

Choose a reason for hiding this comment

WoH commented Oct 3, 2019 • edited Loading

dgreene1 commented Oct 3, 2019

WoH commented Oct 3, 2019

WoH left a comment •

edited

Loading

dgreene1 commented Oct 3, 2019 •

edited

Loading

WoH commented Oct 3, 2019 •

edited

Loading

WoH commented Oct 3, 2019 •

edited

Loading

WoH left a comment •

edited

Loading

WoH commented Oct 3, 2019 •

edited

Loading