[typescript-angular] Added support for Angular v6 (by adding support for RxJS v6) #8155

akehir · 2018-05-08T16:29:26Z

PR checklist

Read the contribution guidelines.
Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if 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\.
Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Update the .mustache templates for Angular 6 (by checking updating RxJS to v6 where appropriate). Also added test cases for Angular 5 & Angular 6, generated tests for Angular 6.

Travis CI: https://travis-ci.org/akehir/swagger-codegen/builds

@macjohnny

…rfaces

macjohnny

awesome, thanks for the PR! I was about to implement this as well, but you were way faster ;-)
I added some small notes that are easy to fix.
Remark: The only place where the rxjs operators imported in https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/resources/typescript-angular/rxjs-operators.mustache are used is here: https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/resources/typescript-angular/api.service.mustache#L89

And since this line is only used for Angular <4.3, it should be safe to apply the changes I request.

macjohnny · 2018-05-09T05:49:26Z

modules/swagger-codegen/src/main/resources/typescript-angular/package.mustache

    "zone.js": "^0.7.6",
-    "typescript": "^2.1.5"
+    "typescript": "~2.7.2"


since we are requiring angular cli, constraining to "typescript": "^2.1.5" should be fine and more flexible, especially when choosing between angular 5 or 6

The problem is, with ^2.1.5, typescript 2.8.3 will be installed during the tests which is not compatible to angular core. I'll change it to >= 2.1.5 < 2.8, would that be more appropriate?

yes, this sounds better

macjohnny · 2018-05-09T05:49:51Z

modules/swagger-codegen/src/main/resources/typescript-angular/api.service.mustache

+{{/useRxJS6}}
+{{#useRxJS6}}
+import { Observable }                                    from 'rxjs';
+{{/useRxJS6}}
 {{^useHttpClient}}
 import '../rxjs-operators';


the import '../rxjs-operators'; should be moved inside the {{^useRxJS6}}{{/useRxJS6}} block, see my general review comment.

Sorry, if I misunderstood, the rxjs-operators is imported in a {{^useHttpClient}} {{/useHttpClient}} - I (wrongly?) thought, this meant that these imports are only used if angular is older than 4.3, and that after 4.3 they are not imported anymore.

For RxJS 6.1, the operators would need to be imported differently, if they are used - see: https://github.com/ReactiveX/rxjs/blob/master/MIGRATION.md#howto-convert-to-pipe-syntax .

From what I can see, actually, of the operators, throw & catch are not used by the api service, only map is. However I just found out, that at least at one point map is used, which will need to be converted to a .pipe( map( ()=> {}), ) for rxjs 6. Let me update the PR and get back to you.

ah, you are right, I missed that it is in a {{^useHttpClient}}{{/useHttpClient}} block.

you don't need to convert the .map() to .pipe(map()), since it is only used is here: https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/resources/typescript-angular/api.service.mustache#L89

And since this line is only used for Angular <4.3, it should be fine as it is

My bad, I misread. You want to move it to the {{^useRxJS6}}{{/useRxJS6}} block, which makes sense. I'll update the PR.

The .map is only used in a {{^useHttpClient}} {{/useHttpClient}} block, so not relevant to angular 6.

actually I think we shouldn't move it to the {{^useRxJS6}}{{/useRxJS6}} block, since this would also affect Angular >=4.3 and <6

After moving it, angular 5 test fails =(

yes, that's what I thought would happen. so you can simply ignore the initial review comment that stated

the import '../rxjs-operators'; should be moved inside the {{^useRxJS6}}{{/useRxJS6}} block, see my general review comment.

macjohnny · 2018-05-09T05:51:00Z

samples/client/petstore/typescript-angular-v2/npm/package.json

@@ -34,7 +34,7 @@
    "reflect-metadata": "^0.1.3",
    "rxjs": "^5.4.0",
    "zone.js": "^0.7.6",
-    "typescript": "^2.1.5"
+    "typescript": "~2.7.2"


this is not good, we should not require typescript 2.7here

macjohnny · 2018-05-09T06:51:55Z

cc @TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx

macjohnny · 2018-05-09T08:24:16Z

samples/client/petstore/typescript-angular-v2/with-interfaces/README.md

@@ -52,7 +52,7 @@ In your Angular project:

 ```
 // without configuring providers
-import { ApiModule } from '@swagger/angular2-typescript-petstore';
+import { ApiModule } from '';


why does the package-name disappear here?

I'm not sure about this one, I believe the package name should not disappear - but I was unfortunately not proficient enough to find the root cause of this. This behaviour is probably not caused by my pull request, though.

The same happens, if I run ./bin/typescript-angular-petstore-all.sh on swagger-codegen/master. Was it incorrect to generate all angular examples with my updated .mustache templates?

Notabene, the same happened here: https://github.com/swagger-api/swagger-codegen/pull/8102/files#diff-ad35a6b2374dd58b5781c610802426c4

@akehir I remember that the package name appears and disappears frequently, so I think it is a problem somewhere else, certainly not cause by your PR. If I generate the samples on Windows, the package name appears. Maybe this is the difference?

macjohnny · 2018-05-09T08:28:45Z

samples/client/petstore/typescript-angular-v2/with-interfaces/api/user.serviceInterface.ts

@@ -11,8 +11,7 @@
 */
 import { Headers }                                           from '@angular/http';

-import { Observable }                                        from 'rxjs/Observable';
-
+import { Observable }                                    from 'rxjs/Observable';


did you re-generate the v2 sample files? this line should not change...

I re-generated by using ./bin/typescript-angular-petstore-all.sh . The padding was different in the apiInterface.mustache, I just committed a fix.

macjohnny

looks good to me

…terfaces readme

keradus · 2018-06-26T18:02:30Z

any chances to have this change release? :)

degahr · 2018-08-02T08:31:10Z

Seems that this issue is linked to 2.4.0 ? What is up with this.. Are we going to see progress on this anytime soon?

macjohnny · 2018-08-13T07:46:33Z

@degahr these changes are included in the latest release of https://github.com/openapitools/openapi-generator

rainmanhhh · 2018-09-22T06:08:07Z

how to pass useRxJS6 flag to the template? config-help doesn't show it

keradus · 2018-09-22T08:18:21Z

java -jar openapi-generator-cli.jar generate -i swagger.json -l typescript-angular --additional-properties ngVersion=6

rainmanhhh · 2018-09-22T11:13:55Z

java -jar openapi-generator-cli.jar generate -i swagger.json -l typescript-angular --additional-properties ngVersion=6

doesn't work, both 2.3.1 and 3.0.0-rc1 generate
import { Observable } from 'rxjs/Observable';

keradus · 2018-09-22T12:26:58Z

switch to openapi, I'm using 3.0.2 and it generates
import { Observable } from 'rxjs';

wget -nc http://central.maven.org/maven2/org/openapitools/openapi-generator-cli/3.0.2/openapi-generator-cli-3.0.2.jar -O openapi-generator-cli.jar

akehir added 3 commits May 8, 2018 17:01

Change RxJS imports, if using Angular >= 6

887afa8

Add testcases for Angular v5 and v6

d1328f5

Remove unwanted whitespace in padding, and restore petstore with inte…

e6bc740

…rfaces

macjohnny suggested changes May 9, 2018

View reviewed changes

Update according to review of @macjohnny

03b955b

macjohnny reviewed May 9, 2018

View reviewed changes

macjohnny approved these changes May 9, 2018

View reviewed changes

Correct padding for typescript v2 test, and revert typescript with in…

e0a5019

…terfaces readme

wing328 added the Client: TypeScript label May 10, 2018

wing328 added this to the v2.4.0 milestone May 10, 2018

wing328 added the Enhancement: Feature label May 10, 2018

wing328 merged commit a004b19 into swagger-api:master May 11, 2018

macjohnny mentioned this pull request May 14, 2018

[typescript-angular] upgrade to support angular v6 (breaking changes due to rxjs v6) #8179

Closed

macjohnny mentioned this pull request Oct 1, 2018

Angular Typescript rxjs import change #8754

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[typescript-angular] Added support for Angular v6 (by adding support for RxJS v6) #8155

[typescript-angular] Added support for Angular v6 (by adding support for RxJS v6) #8155

akehir commented May 8, 2018

macjohnny left a comment

macjohnny May 9, 2018

akehir May 9, 2018

macjohnny May 9, 2018

macjohnny May 9, 2018 •

edited

Loading

akehir May 9, 2018 •

edited

Loading

macjohnny May 9, 2018

akehir May 9, 2018 •

edited

Loading

macjohnny May 9, 2018

akehir May 9, 2018

macjohnny May 9, 2018

macjohnny May 9, 2018

macjohnny commented May 9, 2018

macjohnny May 9, 2018

akehir May 9, 2018 •

edited

Loading

macjohnny May 9, 2018

macjohnny May 9, 2018

akehir May 9, 2018

macjohnny left a comment

keradus commented Jun 26, 2018

degahr commented Aug 2, 2018

macjohnny commented Aug 13, 2018

rainmanhhh commented Sep 22, 2018

keradus commented Sep 22, 2018

rainmanhhh commented Sep 22, 2018

keradus commented Sep 22, 2018

[typescript-angular] Added support for Angular v6 (by adding support for RxJS v6) #8155

[typescript-angular] Added support for Angular v6 (by adding support for RxJS v6) #8155

Conversation

akehir commented May 8, 2018

PR checklist

Description of the PR

macjohnny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macjohnny May 9, 2018 • edited Loading

Choose a reason for hiding this comment

akehir May 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akehir May 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macjohnny commented May 9, 2018

Choose a reason for hiding this comment

akehir May 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macjohnny left a comment

Choose a reason for hiding this comment

keradus commented Jun 26, 2018

degahr commented Aug 2, 2018

macjohnny commented Aug 13, 2018

rainmanhhh commented Sep 22, 2018

keradus commented Sep 22, 2018

rainmanhhh commented Sep 22, 2018

keradus commented Sep 22, 2018

macjohnny May 9, 2018 •

edited

Loading

akehir May 9, 2018 •

edited

Loading

akehir May 9, 2018 •

edited

Loading

akehir May 9, 2018 •

edited

Loading