[JavaScript] Generator options and template improvements

[demonfiddler]

This is to propose the following changes to the JavaScript code generator:

1. RENAME omitModelMethods [=false] to emitModelMethods [=false]. The rationale is two-fold: firstly to suppress their emission by default, since getter and setter methods are not a typical JavaScript programming idiom; their presence bloats code and adds little or no value. Even if it were felt desirable to access properties via a function call, ECMA 5.1 provides for transparent property get set accessor functions. Secondly, the new option name would be consistent with option 2 below.
2. ADD emitJSDoc [=true] so that JSDoc comments are emitted by default but disabled if desired. The JSDoc tags and format to be carefully chosen to conform to the requirements of both JSDoc (for API documentation generation) and Tern.js (to support content assist and validation).
3. ADD useInheritance [=true]. By default generated classes for 'allOf' composed types would use idiomatic JavaScript inheritance via prototype chains for the 'parent' axis and mix in any additional types from the 'interfaces' axis. When set to false, all composed types would be mixed in as at present.
4. REMOVE the toJson() [sic] method from the template, as it bloats code, serves no useful purpose and causes confusion with the toJSON() method recognised by JSON.stringify(). See Move common functionality to new model base class #2075, JS client: Model methods should be added to prototype #2044.
5. MOVE model methods to the prototype. See JS client: Model methods should be added to prototype #2044.
6. DEFINE prototype properties for those owned properties with default values, rather than adding them to the instance as at present. Adding such properties to the instance causes their values to be serialised to JSON, thus defeating the purpose of default values and furthermore making it impossible to restore the default value without a priori knowledge of the default value! Defining the default property value on the prototype allows the default to be restored by simply deleting the instance property.

I have most of the above implemented and when complete I should like to submit a PR. The changes will include unit and functional tests as appropriate.

Comments invited.

[wing328]

@demonfiddler thanks for the suggestion.

cc @delenius @xhh to see if they've any comment.

[xhh]

These improvements look very good to me 👍

[delenius]

(1) and (2) sound good to me.

I don't know what 'parent' vs 'interface' axis means... Does this take care of #2041 ?

@demonfiddler, @xhh In the interest of cleanup and de-bloating, please also comment on #2075

[demonfiddler]

@delenius thanks for your response.

The 'parent' vs 'interface' axes refer to the so-named properties of io.swagger.models.ComposedModel. Such a model may notionally 'extend' a parent (defined to be the first RefModel in the allOf list) and also potentially 'implement' any number of interfaces (defined to be the second and subsequent RefModels in the allOf list). It may also have a child model (defined to be the last ModelImpl in the allOf list), representing the owned properties (see swagger-parser Issue # 204 "allOf deserialised incorrectly").

Currently almost all Swagger code generators (including JavaScript) implement these various forms of inheritance by mix-in composition (i.e., inherited properties are redefined locally). Proposal 3) is to extend the CodegenModel with sufficient information to enable templates to leverage language-specific idiomatic extension and inheritance mechanisms. For JavaScript in particular, the template would set up a prototype chain and call existing inherited model constructor and deserialiser logic rather than replicating it (thus avoiding code bloat). I have already implemented these changes - just need to create some tests.

Regarding #2041, the above proposal does not directly address this. From what I can see, further changes would be necessary to CodegenModel and possibly Model as well, since the 'discriminator' field does not appear to be represented. I propose we address this enhancement separately. I'd be prepared to contribute a solution but not as part of these enhancements - I need to limit the scope and effort as I'm under time constraints.

I'll comment directly on #2075.

[demonfiddler]

Commenters: please also review proposals 4) 5) and 6) in the updated description.

[delenius]

Ok this is a lot of stuff in one issue. Can we split this issue up into a number of separate issues, with separate PRs?

Some comments anyway,

(3) - so, if you have only one type in allOf, it will use prototype inheritance, and if you have more than one, it uses the more "manual" approach? Could this be confusing to users? If you print an object using console.log, some methods will appear under prototype and some other the object.

Where is the documentation for "parent" vs "interface" (I have to say the swagger spec documentation is not very good or easy to browse).

It would be good to have some code examples of what things would look like.

Do the changes to the language-independent parts affect the generation of the other-language clients?

(4) - I'm obviously ok with this too since I have a PR pending. I will rebase it so we can merge it.

(5) - This has already been done. Make sure you're on the latest master.

(6) - Again, some code examples would be great, but again, can we please split things up? I realize things depend on each other, so some PRs will have to wait for others to be merged, but that's normal...

[demonfiddler]

My assigned task is to come up with a usable JavaScript API for our services, and these changes are all necessary to achieve that. I'm afraid splitting this stuff up wouldn't be at all practicable, since the changes are within the same files. I've already split the changes to make them easier to assimilate and I'm now taking heat for the length of time it's taking to get them ready to commit - refactoring this into yet further separate commits and thereby introducing further delays to isn't a viable option for us.

Re. 3.) all I can say is that the single inheritance case is more common than the multiple inheritance case, and that prototypical inheritance is the expected idiomatic JavaScript implementation. Same goes for Java, whose answer to multiple inheritance is interfaces (although I don't propose to make any changes to the Java generator). I don't find this particularly confusing - it's standard practice for code generators (EMF for example). Other languages such as C++ which do support true multiple inheritance would now be able to do so. There's no Javadoc on the relevant Swagger Java source (actually, there's no Javadoc to speak of anywhere!!! :-( ) but it's clear that ComposedModel.parent, interfaces and child fields are populated from allOf by SwaggerDeserializer. If someone doesn't like prototypical inheritance and/or mix-in interfaces, they can set useInheritance=false and revert to the old way of doing things.

Changes to the language-independent parts won't affect generators for other languages, since a.) these changes will only be activated by setting DefaultCodegen.supportsInheritance = true and b.) the CodegenModel changes are additive - all existing fields retain the identical semantics when useInheritance=false. Currently only one language (TypeScript) already does this, but it isn't able to make proper use of it at present because SwaggerDeserializer is broken in its treatment of allOf (see swagger-parser Issue #204). The JavaScript code generator's useInheritance option sets DefaultGenerator.supportsInheritance.

Re. 4), yes, I saw you had this pending.

Re. 5.) I'll sync from master

Re. 6.) I'll endeavour to produce a sample YAML and JavaScript implementation for you to review. I know this looks like a lot when described like this but I'm sure that once you see the generated code that everything will become clear and hopefully highly desirable.

[delenius]

Ok, sounds good, as long as you can get @wing328 to accept your PRs :) Note that the handling of optional parameters was recently changed as well, so make sure that optionals with defaults still works after your changes.

[wing328]

For optional parameters with default value, we do not support that in other API client because the default value should be handled by the server (as the server should not assume the API clients always send request with the proper default value).

[demonfiddler]

My initial changes are ready to commit and I've rebased them to the latest (2016-03-04) HEAD. I'm awaiting consent from our Legal department before I submit a PR. In the meantime, if anyone would like to review the output (JavaScript and JSDoc), I've zipped it all up to illustrate the codegen options described above. I was unable to attach the ZIP files to this comment, just get the message "Unfortunately, we don’t support that file type. Choose Files Try again with a PNG, GIF, JPG, DOCX, PPTX, XLSX, TXT, PDF, or ZIP."

BTW the JSDoc tags introduced by the optional operation parameter handling changes were illegal - JSDoc aborts with a RegExp exception! In fact, if anyone's ever run JSDoc against the generated JavaScript, they would have seen that even prior to this breakage, JSDoc didn't produce any useful output at all, on account of the necessary JSDoc tags being missing or incomplete!

Anyhow, I have corrected these errors in the templates and with my changes JSDoc will actually work:-) I haven't yet looked at Tern.js support - I may need to make further template changes to make the JavaScript compatible with the Tern.js 'condense' utility.

If anyone wishes to review the output, please let me know your e-mail address and I'll send you the ZIP files.