Various fixes for JS (ES6) generator #180
Conversation
|
@delenius thanks for the fixes and enhancements. What about including the the JS Petstore ES6 clients in the Travis test by adding the folders to https://github.com/OpenAPITools/openapi-generator/blob/master/pom.xml#L861 so that the CI will run the tests for us moving forward? |
|
@wing328 , sure! A couple of questions:
|
|
BTW, item (7) in this PR may need more discussion. Previously, the model classes generated calls to superclass constructors, e.g. in Cat.js you would get Animal.call(_this, className);This appears twice before this PR, because it was generated both for the "#parentModel", and for all the "#interfaceModels". It turn out that the former is always (at least in cases I've seen) part of the latter. So I removed the former. Not sure if this causes problems in some other case. If so, we'll need to go deeper and figure out how to filter this some other way (and ideally add more unit tests that expose the difference; currently, it passes). I also had to change those calls because the constructor is no longer a function in es6 (that's issue (4) above), so now you'll see this instead: Animal.initialize(this, className); |
Yes
I don't think it's used but plesae still set it to javascript for consistency. (my apologies for late reply) |
I would suggest going with your approach to move forward with this PR. |
1. Don't use property initializers The ES6 Javascript generator used property initializers, as described [here](https://tc39.github.io/proposal-class-public-fields/). This is not standard ES6. In particular, create-react-app chokes on it (see [this issue](swagger-api/swagger-codegen#8024), which is fixed by this commit). This commit instead reverts back to using prototypes, as in the non-es6 JS generator. 2. Add missing pom.xml files This allows us to run `mvn integration-test` in these folders. 3. Add missing double-quotes around enum field names. This avoids errors due to fields that contain illegal characters. 4. Fix support for multiple inheritance The previous implementation was broken. I added static "initialize" methods, which supports multiple inheritance. 5. Comment out some broken tests. There were tests for OuterBoolean etc, which are not generated as part of index.js in the generated petstore clients. Hence, these tests threw an error. The actual tests were already commented out anyway. I just commented out one more line to avoid an error. 6. Remove some empty lines in generates JS files.
In the generated api files, primitive types did not have single quotes around them, as expected in ApiClient.js (convertToType method). I fixed this by adding a x-return-type vendor extension property (because other parts of the code need it without the quotes). I also fixed some more issues in the unit tests for the JS petstore examples.
This restores the handling of special names as properties, e.g. obj['byte'] = _byte; which was temporarily broken due to previous refactoring.
delenius
force-pushed
the
feature/es6-fixes
branch
from
9c21f44
to
a281ec8
Compare
|
Ok, did that, rebased against latest master, and rebuilt the samples.
…On Thu, Jun 14, 2018 at 10:17 AM, William Cheng ***@***.***> wrote:
Should I add them both under profiles, and also under modules?
Yes
Should the env variable under the profile be javascript for all variants?
I don't know what this does.
I don't think it's used but plesae still set it to javascript for
consistency.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#180 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHkajUAH4k0YBzUg0AJ3rKZzt7Aowvr3ks5t8pqtgaJpZM4USnKE>
.
|
|
If no one has further feedback/question on this PR, I'll merge it into master over the weekend. |
wing328
changed the title
PR checklist
./bin/to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.shand./bin/security/{LANG}-petstore.shif updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in.\bin\windows\.master.Description of the PR
The ES6 Javascript generator used property initializers, as
described here.
This is not standard ES6. In particular, create-react-app chokes on it
(see this issue,
which is fixed by this commit).
This commit instead reverts back to using prototypes, as in the non-es6
JS generator.
This allows us to run
mvn integration-testin these folders.This avoids errors due to fields that contain illegal characters.
The previous implementation was broken. I added static "initialize"
methods, which supports multiple inheritance.
There were tests for OuterBoolean etc, which are not generated
as part of index.js in the generated petstore clients. Hence, these
tests threw an error. The actual tests were already commented out
anyway. I just commented out one more line to avoid an error.
Remove some empty lines in generates JS files.
Fixed doubly generated superclass initializer calls.
Fixed missing single quotes in returnType in the generated "Api" classes.
The es6 and promise-es6 integration tests now pass (they didn't before).
Furthermore, the generated clients now work with create-react-app (see this open swagger-codegen issue).
Copying JS technical committee: @CodeNinjai @frol @cliffano