Repository navigation
Conversation
|
|
||
| var resultType = isNullable && UnwrappedResultType != "void" && UnwrappedResultType != "null" ? | ||
| UnwrappedResultType + " | null" : | ||
| var resultType = isNullable && UnwrappedResultType != "void" ? |
There was a problem hiding this comment.
shouldnt it be
| var resultType = isNullable && UnwrappedResultType != "void" ? | |
| var resultType = isNullable && UnwrappedResultType != "void" && UnwrappedResultType != optionalReturn |
?
There was a problem hiding this comment.
Quite right. I'll add a test for that on Monday and fix it.
There was a problem hiding this comment.
Ok,so I added two test that assert the returned Promise as being Promise<any> when the return type is a nullable any. So we don't get a redundant Promise<any | null>, as any already "includes" null (as well as undefined).
Was that the intent of your original comment? If so, I had to change your suggestion to not check against UnwrappedResultType != optionalReturn but to verify that UnwrappedResultType is not any.
Does that capture your intention?
Update: The original UnwrappedResultType != "null" guarded a generated result type of null, which likely can't happen. The same holds for undefined as a bare result type. But the any case is precisely what is generated by a controller action that returns object. So I'd argue this is what we should be guarding against. Do you agree?
3cfd660 to
0eee331
Compare
|
The generated return type and runtime value currently disagree. For example, the new nullable-string snapshot declares Could we use your alternative A, with a separate We should add runtime tests alongside the snapshots to verify this. Empty bodies and HTTP 204 also need explicit consideration, since no response content is distinct from JSON I've merged latest |
Thanks for your feedback! I'll gladly,
This might take some time to get done properly. Let's say on the order of a week. |
This is sensible since: a) The `TypeScriptGeneratorSettings` set "Undefined" as the default b) The class `OperationResponseProcessorBase` does not use the setting `_settings.DefaultResponseReferenceTypeNullHandling` on line 283, which would then default to non null returns.
…Model.cs Co-authored-by: Rico Suter <mail@rsuter.com>
546d042 to
0cc4849
Compare
atmoos
left a comment
There was a problem hiding this comment.
Hello @RicoSuter
I finally got round to commenting on the new changes. Please see inline.
There is still a pending decision when the API does not allow null as a return value. Now that NSwag can honour the contract, should we treat a 204 as error (a breaking change, but honouring the API) or should we ignore the 204 (ensuring backward compatibility, but not honouring the API)
I marked those cases with "Needs architectural decision".
The other comments where added to point you to the (IMHO) relevant changes or add a reminder for myself, for minor things I noticed while going through the PR. (I plan to update this shortly)
| public string ResultNullValueReturn => | ||
| _settings.ResponseNullValue == TypeScriptNullValue.Undefined && IsNullableResult | ||
| ? ResultNullValue | ||
| : ResultNullValue + " as any"; |
There was a problem hiding this comment.
FYI: This "as any" path is what keeps the behaviour the same when null value has been configured as Null (i.e. not Undefined)
This pattern occurs repeatedly elsewhere, too.
|
|
||
| /// <summary>Gets or sets the value used to represent "no value" in response types and their runtime | ||
| /// conversion, i.e. whether a nullable response is typed and returned as 'null' or 'undefined' (default: Null).</summary> | ||
| public TypeScriptNullValue ResponseNullValue { get; set; } |
There was a problem hiding this comment.
As discussed, a "null" value setting was added that targets return values only. Hence, the original null value settings scope is unaffected by this change.
| Resolver = _resolver, | ||
| Settings = Settings.TypeScriptGeneratorSettings, | ||
| NullValue = TypeScriptNullValue.Null, | ||
| NullValue = Settings.ResponseNullValue, |
There was a problem hiding this comment.
@atmoos To check whether this is correct, or whether we should have added a second ResponseNullValue property.
| {% elsif Framework.IsAngular -%} | ||
| {% if operation.WrapResponse -%} | ||
| return {{ Framework.RxJs.ObservableOfMethod }}<{{ operation.ResultType }}>(new {{ operation.ResponseClass }}(status, _headers, null as any)); | ||
| return {{ Framework.RxJs.ObservableOfMethod }}<{{ operation.ResultType }}>(new {{ operation.ResponseClass }}(status, _headers, {{ operation.ResultNullValue }} as any)); |
There was a problem hiding this comment.
@atmoos To check whether this "as any" remnant is correct.
There was a problem hiding this comment.
The most relevant changes to the liquid files live here.
As before, the hardcoded "as any" part has been replaced to a programmatically created value based on the new null value configuration.
|
|
||
| return result200; | ||
| }); | ||
| } else if (status !== 200 && status !== 204) { |
There was a problem hiding this comment.
Return Case: "empty means a configured {undefined}"
=> A 204 is a valid response => Not throwing, but return undefined.
|
|
||
| return result200; | ||
| }); | ||
| } else if (status !== 200 && status !== 204) { |
There was a problem hiding this comment.
Return Case: "empty means a configured {null}"
=> A 204 is a valid response => Not throwing, but return null.
| result200 = ReturnDto.fromJS(resultData200); | ||
| return result200; | ||
| }); | ||
| } else if (status !== 200 && status !== 204) { |
There was a problem hiding this comment.
Return Case: "empty is not part of the API, the configured {undefined} setting case"
=> A 204 is not a valid response as per API => But, don't throw for backward compatibility and return undefined regardless.
@RicoSuter: Needs an architectural decision
| result200 = ReturnDto.fromJS(resultData200); | ||
| return result200; | ||
| }); | ||
| } else if (status !== 200 && status !== 204) { |
There was a problem hiding this comment.
Return Case: "empty is not part of the API, the configured {null} setting case"
=> A 204 is not a valid response as per API => But, don't throw for backward compatibility and return null regardless.
@RicoSuter: Needs an architectural decision
|
|
||
| return result200; | ||
| }); | ||
| } else if (status !== 200 && status !== 204) { |
There was a problem hiding this comment.
Return Case: "empty is implicitly part of the API, the configured {null} setting case"
=> A 204 is arguably a valid response as per API => return null.
Rationale
Returning no value as a valid response to an endpoint (Controller action) is a viable pattern that is apparently currently only supported via
nullby the NSwag typescript code generator.The apparent configuration value
nullValue = Undefinedis unfortunately not propagated to return types, meaning they are only everTReturn | nullinstead ofTReturn | undefined.Proposal Outline
This PR bridges that gap in the TypeScriptOperationModel, by propagating the already existing setting to return types, enabling the following:
nullValueTReturnTReturnTReturn?TReturn | undefined2)TReturn?UndefinedTReturn | undefinedTReturn?NullTReturn | nullThis is also mirrored in the tests I added.
Also the above table applies not just to C#'s nullability feature but also when using the nullability attributes. Naturally, this actually relies on the OpenApi definition
"x-nullable": "true/false".Notes
The return type irrespective of how it's defined in C# (
TReturn,Task<TReturn>,{I}ActionResult<TReturn>). The OpenApi definition is relevant.The fact that the default value for
nullValueisUndefinedis arguably the right choice from a TS point of view. In the context of this change it is possibly somewhat unfortunate for backward compatibility. But see the discussion about alternatives further below.Alternatives
This PR represents the (arguably) simplest possible way to enable undefined returns. It may not be the optimal solution in terms of backward compatibility (point 2)). Hence, here some Ideas to change the implementation, if so desired by the maintainer.
A) Additional Setting
Add another independent setting: say a
nullReturnValuewhich would take exactly the same values asnullValue, where the default would be set toNullfor backward compatibility.B) Changing the default value of
nullValuetoNullThis would keep return types the same, but might wreak havoc on input types and Dto parameters. If this PR were merged with a changed default value, any project that does not set that value would suddendly be seeing
TInputParameter | nullinstead ofTInputParameter | undefined. (Although by setting thenullValuesetting this would be easily fixable).Tests
It may seem that there are too many changed tests. Here's why they changed
TReturn(some class) would be interpreted as nullable (consistent with C#'s default behaviour prior to the nullability feature)_settings.DefaultResponseReferenceTypeNullHandlingis added to the configuration of thetypeDescriptoron line 283 in OperationResponseProcessorBase.cs, the interpretation would change to "NotNull". This would lead to changes in the test verifications formTReturn | nulltoTReturnonly