Discuss changes for TypeScript strict checks

[grovesNL]

Just wanted a place to document and discuss any changes we consider in the TypeScript clients as we add stricter checks in case the context behind the original types were missed.

Firstly consider the return null; within a method expecting a return type of void. Do we intentionally return null here (in which case we should update the return type) or could this statement simply be return; instead? If this could be clarified I'd be happy to make the change, but I want to avoid breaking changes if null is actually expected.

I found where return null; was added to try to gain some context. It was added in
0edf241#diff-1e10b65d13165f5da1c352bfcbceba19 but I can't tell whether this is the desired behavior.

[grovesNL]

@RSuter Based on the above, could you take a look at master...grovesNL:typescript-type-improvements when you have some time and let me know your thoughts? I can open a PR but I consider it experimental for now.

[RicoSuter]

The return type must match the generated method signature (ie the signature wins).

[grovesNL]

@RSuter But in some cases the signature that was used was impossible. i.e. processGetAll claims to have a return type of Parent[] but that's only true for one case. In several other cases it will return null, so the return type should be Parent[] | null.

Likewise because this return type is used in getAll, ng.IPromise<Person[]> isn't true either. In fact there is another branch where there is no explicit return so that one is undefined. The new signature must then become ng.IPromise<Person[] | null | undefined>, unless we change it to return null; explicitly which will narrow it to ng.IPromise<Person[] | null>. But there is still a null case regardless.

[RicoSuter]

The whole strict null thing is not yet implemented anywhere... We need to fix the method headers (with "| null" as you suggested) and also fix the "return null;" if it is wrong. But it looks like the method signature is wrong in this case...

The we need to add "| null" to the return types which are nullable but only if strict null is supported (TS >= v2.0?) I think this must be done in NJsonSchema

[grovesNL]

@RSuter Yep exactly, that's what I'm attempting in https://github.com/grovesNL/NSwag/tree/typescript-type-improvements (you can see all the added return null and modified return types). I'm just not sure whether I should return null; explicitly or allow it to return undefined.

There are some changes that have to happen in NJsonSchema for the conversion code but the majority will have to happen on the NSwag side. If you look at my commit messages in the referenced URL, I have the majority of the strict options (like strictNullCheck) working with these updated return types/other trivial changes. There are only a few errors that remain.

[grovesNL]

@RSuter Let me know whether you think it's worthwhile to make these changes. If you're interested, I'll try to merge the recent template changes from master so my branch doesn't fall too far behind

[RicoSuter]

NJS also updated for strict null checks: RicoSuter/NJsonSchema@a561d72

[RicoSuter]

I think your changes are worth it. But we have to be very careful (no breaking changes, support for TS 1.8).

I've seen Promise<void | null>: I think this promise type should be avoided (i.e. in this case return undefined and use Promise)

Can you create a list of the changes?

[grovesNL]

@RSuter Sounds good.

For the Promise<void | null> should we use Promise<void | undefined> if strictNullChecks are enabled and Promise<void> otherwise (i.e. TS 1.8)?

As I mentioned above, there are some places where null is returned explicitly. In this case the return type for the caller may look like Promise<string[] | null | undefined>. In this case should we removed the return null; and just return undefined instead? Then the promise becomes Promise<string[] | undefined> (TS >2.0) or Promise<string[]> (TS 1.8)

My changes should be documented with each commit message but I can add the list here as well. I also need to add flags to support TS 1.8 wherever undefined will be used. After we make the improvements for strict checks it would be good if we could establish some unit/integration tests for all of the TS clients.

[grovesNL]

Also great job adding support for strict null checks to NJS. After we add the changes I mentioned here, we should only have a couple "strict options" issues left.

[RicoSuter]

For the given example: The problem is, that we don't know whether the response is nullable or not. In the Swagger spec, there is no means of specifying if "null" or "undefined" is allowed (the JSON Schema type "null" is not allowed and there is no property "required" or "optional"). NSwag generates the property "x-nullable" for that which can be evaluated with the response.IsNullable(...) method...

The question: Should we support "undefined" as response?

I'd say no, then we should use the following rules:

• With response: If the response.IsNullable(handling, true) then: Promise<Person | null> (second param: if "x-nullable" is not defined, then the fallback is true), Promise<Person> otherwise
• Without response (i.e. response.ActualResponseSchema == null): I'd say we use Promise<void | null> but is there no better way to describe a Promise without result (i.e. only Promise?)

[grovesNL]

With response:
I think Promise<string[] | null> when its nullable makes sense. However when we use a return type of Promise<Person> we can't return null; anymore. Because we return undefined by default then, the promise signature becomes Promise<string[] | undefined> (TS >2.0) or Promise<string[]> (TS 1.8). Is that correct?

Without response:
Because Promise is generic we can't use just Promise unless this is handled somehow in the type definition (i.e. the recently merged TS proposal for optional generics). So I guess the signature would be either Promise<void | null> or Promise<void | undefined> for now. I think I would prefer Promise<void | undefined> rather than explicitly having to return null though. I'll look into it some more to see what other projects are doing for this return type.

[RicoSuter]

Maybe we should use

• Promise<string[] | null | undefined> when its nullable
• Promise<string[] | null> when its not nullable
• Promise<void | undefined> when it has no response type (or better: Promise<undefined>?)

Meaning:

• null: The server may return null
• undefined: An unexpected but successful response received (HTTP status code not defined in Swagger, ie the default return null;; must be changed to return undefined; - or removed)

This way the generation is simpler (you don't have to check whether to generate the default return null and return undefined) and the caller can check whether the server actually sent null or just an accepted response...

What do you think?

[grovesNL]

I think those signatures are fine from the caller's perspective. It's obvious from the return type what actually happened if you want to handle null differently.

Although I'm not sure if the undefined in Promise<string[] | null | undefined> matters anyway because throwException should handle any issues in the undefined case. So maybe it does make sense to narrow it to Promise<string[] | null> and keep the return null; to simplify the checking the caller will have to do. For example in the case of Promise<int | null | undefined>, because zero is falsely it could be slightly confusing. I'm not sure.

If we want to support NullValue like NJS in the future the Promise<string[] | null | undefined> and Promise<string[] | null> signatures would both become Promise<string[] | undefined>. Would that be okay?

For the note about Promise<undefined>, it seems like Promise<void> is the preferred way to indicate no return type for a promise. Promise<undefined> sounds like we call resolve(undefined). In practice they should essentially behave the same so this should be fine.

[grovesNL]

@RSuter I updated my branch slightly and merged in your latest changes. The diff is relatively clean but the changes are basically:

• Removed many explicit return types because TypeScript should be able to infer the correct types automatically. If there are cases where the return type has changed, it means there were undocumented return types that the callsites wouldn't know about.
• Added explicit return null; where type signatures already had code paths where null could be returned. So rather than Promise<string[] | null | undefined> (where return undefined; happened implicitly) the type has been narrowed to Promise<string[] | null>.
• Added imports for AngularJS and Knockout at the top of the file. This corresponds to the updated @types imports because the types are not global/ambient anymore.
• Changed AngularJS http: ng.IHttpService | null = null; to http: ng.IHttpService; because http is always assigned in the constructor. This was done to remove errors throughout the generated code due to the possibility of http being null.
• Added _ prefix to parameters which may be unused. This excludes parameters from noUnusedParameters checks (see more here). It's relatively difficult to determine whether a parameter will be used in generated method bodies, so many of the parameters have been prefixed.

With these changes, almost all of the templates satisfy the strict checks. You can compile the TS clients with the new release mode (ReleaseTypeScriptStrict) to see where the remaining errors occur.

The remaining errors should only occur in noUnusedLocals and NJS client transformation code. The noUnusedLocals may be hard to fix because it detects unused locals like this one:

const _status = _response.status;
{
    return null;
}

We could do a simple string match or similar here to check if _status is used in the generated code, but that's not very elegant. For the NJS client transformation code, I assume these will be corrected when we update to a newer NJS version or whatever is required to enable NJS strict checks.

Let me know what you think. I'll send a PR if you're fine with the changes.

[RicoSuter]

This looks good to me... can you create a PR for the latest master commit?

[RicoSuter]

Minor change request: Rename

public bool IsAngular => 

to

public bool IsAngularJS => 

[RicoSuter]

Added imports for AngularJS and Knockout at the top of the file. This corresponds to the updated @types imports because the types are not global/ambient anymore.

What exactly does that mean? I think we may break backward compatibility here... We should add an option for that (default: generate imports) so that the old behavior can be enabled.

Next release will be major (v10) because there are some small breaking changes... I hope to merge your work before releasing.

[grovesNL]

Sounds good, thanks for reviewing.

The new TypeScript type definitions provided by @types modules are not ambient, so you have to import each module's types whenever you need them. So we have to manually import Angular and Knockout types. The @types method is the preferred way of managing types now but you're right, it would likely cause issues if the older tsd etc. methods are used.

I'm traveling today but when I have some time (probably tomorrow) I'll rename the property, add the import option to preserve backwards compatibility and send a PR.

[RicoSuter]

Should we call it ImportRequiredTypes (default: true)?

[RicoSuter]

I think we also need to import JQuery:

import * as jQuery from 'jquery'

for the JQuery templates

[RicoSuter]

Just FYI: NJsonSchema and NSwag are tightly coupled... we can work there as well. I'm the developer of both libraries...

[grovesNL]

@RSuter Yeah I knew that, thanks. Hopefully we shouldn't need to make too many changes there for now.

The jQuery import doesn't seem to be required by now due to the static declaration but we should probably import it to be explicit anyway (in case the definition changes to only export the module). On closer inspection Knockout seems to have a static declaration too, but we may as well keep the import to be explicit.

I think ImportRequiredTypes is fine. I'm planning to merge with master tonight (working through the T4 template diffs) and then I'll add ImportRequiredTypes.

[RicoSuter]

See #497

[grovesNL]

@RSuter Seems like we'll need another option besides ImportRequiredTypes in that case

[RicoSuter]

Ok, now I've got only this error:

C:\Data\Projects\NSwag\src\NSwag.Integration.TypeScriptWeb>tsc
node_modules/@angular/core/src/util/decorators.d.ts(11,5): error TS2411: Property 'extends' of type 'Type<any> | undefined' is not assignable to string index type 'Function | any[] | Type<any>'.

[grovesNL]

@RSuter: Looks like it's this issue: angular/angular#8720

Which version of tsc and @angular/core are installed in node_modules? If you delete the @angular/core folder in node_modules and do npm install does the error remain? I'll see if I can reproduce after I pull latest.

Thanks for adding the explicit return types back for those methods.

[RicoSuter]

I also added the MomentJS import...

The only problem now is that fetch/aurelia do not have Access to the Response if the default Response is blob..

[grovesNL]

@RSuter: How is it different than methods that return other objects? i.e. https://github.com/NSwag/NSwag/blob/master/src/NSwag.Integration.TypeScriptWeb/scripts/serviceClientsFetch.ts#L415 Where would the caller be able to access the Response in that case?

[RicoSuter]

You cannot access the response object from outside...

[RicoSuter]

The problem is this:

protected processGetUploadedFile(_response: Response): Promise<Blob | null> {
    return _response.blob().then((_blob) => {
        let _responseText = "";
        const _status = _response.status;
        if (_status === 200) {
            return _blob;
        } else if (_status !== 200 && _status !== 204) {
            this.throwException("An unexpected server error occurred.", _status, _responseText);
        }
        return null;
    });
}

As you can see, in the exception case, the responseText is "" (we have to completely change the templates to support that)

[grovesNL]

Oh, you mean when it's passed to throwException?

I suppose we could check the default status before we call _response.blob() and call _response.text() instead, I guess that's what you mean by the template change.

Alternatively we could probably parse the _blob as a string for those cases. I haven't worked much with Blob so I don't know if this can be done efficiently.

A third option is to clone the stream earlier and use that, but that's not very efficient.

[RicoSuter]

I think the best option is a) and read blob or text inside the "if status code", but this will require some changes... Blob to text is not simple and clone is to expensive

[RicoSuter]

I will look into this today or tomorrow... Otherwise i think we are good? Are you doing some integration tests with the latest bits? Do you think we need an option to disable blob handling and use the previous (any) behavior?

[grovesNL]

I haven't done any integration tests yet but I'll try to do this soon if I have some time. Functionally I don't think anything should have changed so it should be fine. I'm not using blobs so that change wouldn't affect me.

[grovesNL]

Everything discussed in this issue has been resolved as far as I know - closing.