[soap] Update SOAP definitions #18511

onury · 2017-07-30T16:47:56Z

Please fill in this template.

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Follow the advice from the readme.
Avoid common mistakes.
Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes: https://github.com/vpulim/node-soap
Increase the version number in the header if appropriate.
If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

Changes

Server and Client are now exported as classes instead of interfaces. This allows proper inheritance.
Added missing soap module exports: WSDL, WSSecurityCert, ClientSSLSecurityPFX, passwordDigest
Updated existing signatures for missing arguments or members.
Added interfaces: IWSDLBaseOptions, IServerOptions, IXMLAttribute, IServicePort, IService, IServices.
Semantics:
- Refactored Security to ISecurity (Security is still declared for compatibility reasons) - Refactored Option to IOptions (Option is still declared for compatibility reasons)
- Refactored SoapMethod to ISOAPMethod (SoapMethod is still declared for compatibility reasons)
Updated tests.
Fixes incorrect type definition @types/soap #18497

- `Server` and `Client` are now exported as classes instead of interfaces. This allows extending these classes. - Added missing soap module exports: `WSDL`, `WSSecurityCert`, `ClientSSLSecurityPFX`, `passwordDigest` - Updated existing signatures for missing arguments or members. - Added interfaces: `IWSDLBaseOptions`, `IServerOptions`, `IXMLAttribute`, `IServicePort`, `IService`, `IServices`. - Semantics: - Refactored `Security` to `ISecurity` (`Security` is still declared for compatibility reasons) - Refactored `Option` to `IOptions` (`Option` is still declared for compatibility reasons) - Refactored `SoapMethod` to `ISOAPMethod` (`SoapMethod` is still declared for compatibility reasons)

the new `defaults` parameter is optional.

dt-bot · 2017-07-30T16:47:58Z

types/soap/index.d.ts

to authors (@aleung @CageFox). Could you review this PR?
👍 or 👎?

Checklist

pass the Travis CI test?

CageFox · 2017-07-30T17:01:22Z

Seems good and useful

aleung

I'm still reviewing. Submit the comment about naming convention first.

aleung · 2017-07-31T05:40:39Z

types/soap/soap-tests.ts

@@ -1,18 +1,18 @@
-import * as soap from 'soap';
+import * as SOAP from 'soap';


SOAP doesn't align with naming conventions.

This is ok.

aleung · 2017-07-31T05:44:04Z

types/soap/index.d.ts


-interface Security {
+export interface ISOAPMethod {


I prefer to treat abbreviations as if they were lower case words. See this highest vote answer in SO.

ISoapMethod is more readable than ISOAPMethod.

This comment also apply to WSDL, e.g. IWSDLBaseOptions.

The soap module itself has the followings:

WSSecurity - not WsSecurity

ClientSSLSecurity - not ClientSslSecurity

ClientSSLSecurityPFX - not ClientSslSecurityPfx

Also, the original module exports WSDL not Wsdl

I'm not only following my preferences, this is the original module's style/convention.

I don't think there is a consistent style in node-soap project.

You can also find these:

wsdl_headers

wsdl_options

wsdlOptions

addSoapHeader

changeSoapHeader

soapError

Treating abbreviations as normal words increases readability. For example, a glance at ISOAPMethod might become ISO-AP-Method or IS-OAP-Method.

ok. see final changes. I'll keep WSDL as is, since it's exported all in uppercase in node-soap.

typescript-bot · 2017-07-31T06:31:57Z

@onury Please address comments from the code reviewers.

aleung · 2017-07-31T06:41:48Z

types/soap/index.d.ts

-    setSecurity(s: Security): void;
-    [method: string]: SoapMethod | Function;
+    setSecurity(security: ISecurity): void;
+    [key: string]: any; // [method: string]: ISOAPMethod | Function;


it should be [key: string]: any; otherwise TS will complain on any member you add to the extended class if it doesn't have a type of SoapMethod | Function.

Can you give example code of extended class? I tried to write a sub class without changing it to any and there was no error reported by TSC.

minor revisions. changed back to `[method: string]: ISOAPMethod | Function;`

typescript-bot · 2017-07-31T13:55:06Z

@aleung - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate?

aleung · 2017-08-01T01:15:11Z

@onury Thanks for your update.

Another thing I didn't point out yesterday. But after revisiting the code and doing some reading online, I think this definition should follow TypeScript coding guidelines interface naming rule:

Do not use "I" as a prefix for interface names.

There was a lot of discussion on it, e.g. microsoft/TypeScript-Handbook#121, inversify/InversifyJS#242. I‘m disposed to agree the argument in https://softwareengineering.stackexchange.com/a/117393/42485.

I know style choice is somewhat personal favor. But since TypeScript team chooses this one and most of the files in https://github.com/DefinitelyTyped/DefinitelyTyped follow it, why us not?

onury · 2017-08-01T03:07:44Z

@aleung this is my preference. I'm proposing this PR as is. Let me know if I should close this. Thanks.

aleung · 2017-08-01T03:40:31Z

@onury Thank you very much for your great work. Your PR fills up the missing APIs and makes it complete. I really don't want to see that the effort you pay is wasted.

Keeping unified code style is one of the job that maintainer needs to do. @types/soap was using the no I prefix interface naming style from its beginning. I should raised this comment together with previous ones.

aleung

Do not use "I" as a prefix for interface names.

CageFox · 2017-08-01T08:39:32Z

Use "I" prefix in interfaces or don't use is personal or team choice. As you @aleung mentioned above there is a lot of discussion about it and there is no single "right" point about it. Personally I and my team use "I" prefix for many practical reasons.

So, you trying to force your personal preference upon independent author. For my point of view it looks bad. @onury types is much better then it is now and you rejecting his PR just because, let's talk it so: you didn't like his naming style

aleung · 2017-08-01T15:54:47Z

While existing code were using one style (SoapMethod, Option), changing it to another style in a PR because "this is my preference", isn't it trying to force personal preference upon other authors?

Personally I don't feel using "I" prefix in interfaces is a good idea in TypeScript. As it was said in TypeScript Handbook:

In general, you shouldn’t prefix interfaces with I (e.g. IColor). Because the concept of an interface in TypeScript is much more broad than in C# or Java, the IFoo naming convention is not broadly useful.

interface ISoapMethod {
    (args: any, callback: (err: any, result: any, raw: any, soapHeader: any) => void, options?: any, extraHeaders?: any): void;
}

can be rewritten to:

type ISoapMethod = (args: any, callback: (err: any, result: any, raw: any, soapHeader: any) => void, options?: any, extraHeaders?: any) => void;

Are you saying that it's still an interface?

CageFox · 2017-08-01T17:11:39Z

While existing code were using one style (SoapMethod, Option), changing it to another style

Existing soap javascript code have no interfaces by obvious reasons, this is new entities in soap types. Those new entities may be styled as "without "I" prefix" and "with "I" prefix" and it's personal choice of author

There is no "right" choice wich accepted by all or almost all community, and I think it's wrong to force one style

And yes, interfaces can be rewritten as abstract classes or even types, but this changes nothing: existing soap code not exporting abstract classes nor types, so how to make this new entities is author choice

And one more thing: if there was crowd of authors in entrance with different styled typings we would be able to choose "right" and "good" style. But there is only one PR and it's good PR - except you didn't like it's styling very much

onury · 2017-08-02T21:25:57Z

@aleung Yes, you can write it as an interface or a type. Is it really an interface, you ask...

Well, is it really a type?.. It still enforces certain properties on an object/class which is what interfaces are for. Can you do new ISoapMethod()? or ISoapMethod.property? An interface/type transpiles to "nothing" in JS. Besides, I've used ISoapMethod for an actual interface. What's the purpose of that example?

The coding guide-lines for TypeScript you've mentioned is meant to be for contributors of the TypeScript project on GitHub. See the huge note here? That's not how TS developers should mandatorily write code. I personally adapt most of the suggestions there but not all.

But that's not the point...

I've previously made many changes you've asked, including naming style bec. I've agreed with you on for example, abbreviations to be treated as words for better readability. On interface names, I simply do not agree. But you won't come half-way, will you?

I've given time and effort to complete the typings bec. you said "I had only add definitions for my need". Well, I've added the complete definitions (even the ones I don't use), even fixed bugs, out-dated signatures — for community needs. But It all got stuck with your style authoritarianism.

So, will not wait anymore for your approval. here you go..

onury added 3 commits July 30, 2017 19:31

Update index.d.ts

3c5c57c

the new `defaults` parameter is optional.

Update soap-tests.ts

895fd1f

aleung suggested changes Jul 31, 2017

View reviewed changes

typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Jul 31, 2017

aleung reviewed Jul 31, 2017

View reviewed changes

typescript-bot removed the Revision needed This PR needs code changes before it can be merged. label Jul 31, 2017

onury added 4 commits July 31, 2017 16:10

Update index.d.ts

4473990

minor revisions. changed back to `[method: string]: ISOAPMethod | Function;`

import soap instead of SOAP

747e582

Changes: ISoapMethod, IWsdlBaseOptions, IXmlAttribute

d177d32

Refactor for changes in index.d.ts

dbee5d4

onury added 2 commits August 1, 2017 03:48

Added ISoapFault - supports both 1.1 and 1.2 fault styles

26e7875

Added fault test

4628c1e

aleung suggested changes Aug 1, 2017

View reviewed changes

typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Aug 1, 2017

aleung mentioned this pull request Aug 1, 2017

Add typescript support vpulim/node-soap#955

Merged

sheetalkamat added the Awaiting reviewer feedback label Aug 1, 2017

onury closed this Aug 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[soap] Update SOAP definitions #18511

[soap] Update SOAP definitions #18511

onury commented Jul 30, 2017 •

edited

Loading

dt-bot commented Jul 30, 2017

CageFox commented Jul 30, 2017

aleung left a comment

aleung Jul 31, 2017

onury Jul 31, 2017

aleung Jul 31, 2017

onury Jul 31, 2017

aleung Jul 31, 2017

onury Jul 31, 2017 •

edited

Loading

typescript-bot commented Jul 31, 2017

aleung Jul 31, 2017

typescript-bot commented Jul 31, 2017

aleung commented Aug 1, 2017

onury commented Aug 1, 2017

aleung commented Aug 1, 2017

aleung left a comment

CageFox commented Aug 1, 2017 •

edited

Loading

aleung commented Aug 1, 2017

CageFox commented Aug 1, 2017

onury commented Aug 2, 2017

		@@ -1,18 +1,18 @@
		import * as soap from 'soap';
		import * as SOAP from 'soap';

[soap] Update SOAP definitions #18511

[soap] Update SOAP definitions #18511

Conversation

onury commented Jul 30, 2017 • edited Loading

Changes

dt-bot commented Jul 30, 2017

CageFox commented Jul 30, 2017

aleung left a comment

Choose a reason for hiding this comment

aleung Jul 31, 2017

Choose a reason for hiding this comment

onury Jul 31, 2017

Choose a reason for hiding this comment

aleung Jul 31, 2017

Choose a reason for hiding this comment

onury Jul 31, 2017

Choose a reason for hiding this comment

aleung Jul 31, 2017

Choose a reason for hiding this comment

onury Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

typescript-bot commented Jul 31, 2017

aleung Jul 31, 2017

Choose a reason for hiding this comment

typescript-bot commented Jul 31, 2017

aleung commented Aug 1, 2017

onury commented Aug 1, 2017

aleung commented Aug 1, 2017

aleung left a comment

Choose a reason for hiding this comment

CageFox commented Aug 1, 2017 • edited Loading

aleung commented Aug 1, 2017

CageFox commented Aug 1, 2017

onury commented Aug 2, 2017

onury commented Jul 30, 2017 •

edited

Loading

onury Jul 31, 2017 •

edited

Loading

CageFox commented Aug 1, 2017 •

edited

Loading