Make HttpClient#buildRequest headers and options parameter mandatory
#984
Conversation
| @@ -159,7 +159,7 @@ export function listen(server: any, options: IServerOptions): Server; | |||
|
|
|||
| export class HttpClient { | |||
| constructor(options?: IOptions); | |||
| buildRequest(rurl: string, data: any | string, exheaders?: { [key: string]: any }, exoptions?: { [key: string]: any }): any; | |||
| buildRequest(rurl: string, data: any | string, exheaders: { [key: string]: any }, exoptions: { [key: string]: any }): any; | |||
There was a problem hiding this comment.
I'm a bit confused. They weren't mandatory before. Why should they be mandatory now?
There was a problem hiding this comment.
Why not. The jsdoc does not mark them as optional, and there is only one instance, which does not set them to an empty object like all the others. So if we just set them to an empty object by default in this one instance, we can make the parameters mandatory and do not have to handle a falsy value in this function, thus taking complexity out of it
There was a problem hiding this comment.
Documentation is one thing. If the implementation had them as optional in the past, then I'm of the opinion that we should update the documentation i.e. ts file or jsdoc.
There was a problem hiding this comment.
Is this even a method that most users of node-soap would use? If not, then I'm fine with this change.
| var request_headers = options.wsdl_headers; | ||
| var request_options = options.wsdl_options; | ||
| var request_headers = options.wsdl_headers || {}; | ||
| var request_options = options.wsdl_options || {}; |
There was a problem hiding this comment.
Those are the only instances where the two parameter may become falsy
|
Being that these have been optional for a while, I'm deciding that we shouldn't make them required now. It is possible that others have been interfacing with the HttpClient directly. I updated master so the tests should pass now. If you'd like to update the JSDoc/tsdoc to mark the parameters as optional feel free. |
|
Works for me, thank you! |
With #979 I sadly introduced a bug, because the two parameters are optional, despite the fact that they are not marked as such in the jsdoc comment.
This commit makes them mandatory and should fix the tests.