migration to HttpClient #6951

NilsWild · 2018-01-05T18:26:17Z

see: #6281

Please make sure the below checklist is followed for Pull Requests.
Travis tests are green
Tests are added where necessary
Documentation is added/updated where necessary
Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed

… alert-error component accordingly

…angular/http

deepu105

I had quick look from my mobile, will take a closer look later

deepu105 · 2018-01-05T19:30:42Z

generators/client/templates/angular/src/main/webapp/app/_app.module.ts

 import { PaginationConfig } from './blocks/config/uib-pagination.config';
+import { HTTP_INTERCEPTORS } from '@angular/common/http';


Please move this import above local imports

deepu105 · 2018-01-05T19:32:14Z

generators/client/templates/angular/src/main/webapp/app/_app.module.ts

 import { BrowserModule } from '@angular/platform-browser';
-import { Ng2Webstorage } from 'ngx-webstorage';
+import { Ng2Webstorage<%_ if (authenticationType === 'jwt') { _%>, LocalStorageService, SessionStorageService <%_ } _%> } from 'ngx-webstorage';


Inline template should use <% %> syntax

deepu105 · 2018-01-05T19:33:06Z

generators/client/templates/angular/src/main/webapp/app/_app.module.ts

+import { AuthExpiredInterceptor } from './blocks/interceptor/auth-expired.interceptor';
+import { ErrorHandlerInterceptor } from './blocks/interceptor/errorhandler.interceptor';
+import { NotificationInterceptor } from './blocks/interceptor/notification.interceptor';
+import { JhiEventManager } from 'ng-jhipster';


Please put this import above local imports along with other library imports

deepu105 · 2018-01-05T19:36:54Z

...s/client/templates/angular/src/main/webapp/app/admin/configuration/_configuration.service.ts

    }

    get(): Observable<any> {
-        return this.http.get(SERVER_API_URL + 'management/configprops').map((res: Response) => {
+        return this.http.get('management/configprops', { observe: 'response' }).map((res: HttpResponse<any>) => {


Why is the server api url removed?

deepu105 · 2018-01-05T19:43:38Z

generators/client/templates/angular/src/main/webapp/app/shared/auth/_auth-session.service.ts


-        return this.http.post(SERVER_API_URL + 'api/authentication', data, { headers });
+        return this.http.post(SERVER_API_URL + 'api/authentication', data, { headers: headers});


Why switched from shorthand?

sendilkumarn · 2018-01-06T00:10:56Z

generators/client/templates/angular/src/main/webapp/app/_app.module.ts

+        <%_ if (authenticationType === 'jwt' || authenticationType === 'uaa') { _%>
+        {
+            provide: HTTP_INTERCEPTORS,
+            useFactory: (injector) => new AuthExpiredInterceptor(injector),


Have you checked, this part? (I am not sure)AOT is handling this correctly in the new version...

can't we use useClass here?

sendilkumarn · 2018-01-06T00:15:13Z

generators/client/templates/angular/src/main/webapp/app/account/sessions/_sessions.service.ts

@@ -30,10 +30,10 @@ export class SessionsService {
    constructor(private http: Http) { }

    findAll(): Observable<Session[]> {
-        return this.http.get(this.resourceUrl).map((res: Response) => res.json());
+        return this.http.get<Session[]>(this.resourceUrl);


I really liked this one 👍 It clearly states what is gonna be there ❤️

i tried to type everything i stubled over not sure though if i really did it eveywhere so far

sendilkumarn · 2018-01-06T00:18:40Z

...client/templates/angular/src/main/webapp/app/blocks/interceptor/_auth-expired.interceptor.ts

+        }, (err: any) => {
+            if (err instanceof HttpErrorResponse) {
+                if (err.status === 401) {
+<% if (authenticationType === 'jwt') { %>
                    const loginService: LoginService = this.injector.get(LoginService);


Is there any possibility to remove the injector completely? This looks like hacky, can't we inject these services straight

will try that tomorrow it should at least work for login service not sure what happens with Principal

Ok tried to remove it and it doesnt work as it woudl create a cyclic dependenncy becuase login service uses HttpClient that uses the interceptor

yes, Injector is used to avoid cyclic dependency hell in angular

sendilkumarn · 2018-01-06T00:26:07Z

generators/client/templates/angular/src/main/webapp/app/blocks/interceptor/_auth.interceptor.ts

        }

        const token = this.localStorage.retrieve('authenticationToken') || this.sessionStorage.retrieve('authenticationToken');
        if (!!token) {
-            options.headers.append('Authorization', 'Bearer ' + token);
+            request = request.clone({


Doesn't it mutate the entire headers? If there are any headers set?

no according to this implementation https://github.com/angular/angular/blob/5.1.3/packages/common/http/src/request.ts#L67-L385

setHeaders adds the header. headers would mutate all headers.

badnaming on angular site though

I was confused too... so just wanted to make sure...

sendilkumarn · 2018-01-06T00:34:01Z

generators/client/templates/angular/src/main/webapp/app/shared/user/_user.service.ts

    }

    find(login: string): Observable<User> {
-        return this.http.get(`${this.resourceUrl}/${login}`).map((res: Response) => res.json());
+        return this.http.get(`${this.resourceUrl}/${login}`);


yep missed it here like i said not sure if i added types everywhere

sendilkumarn · 2018-01-06T00:35:54Z

@NilsWild awesome work 👍

NilsWild · 2018-01-06T01:06:17Z

will take a closer look tomorrow 2:05 am now :D

pascalgrimaud · 2018-01-06T09:58:16Z

Yes, impressive work for a 1st contribution, @NilsWild !

deepu105 · 2018-01-12T09:50:59Z

@NilsWild we would love more inconvenience from you then 😺

NilsWild · 2018-01-12T09:51:22Z

Failed: Angular could not be found on the page http://localhost:8080/.If this is not an Angular application, you may need to turn off waiting for Angular.

any idea what may cause this?

sendilkumarn · 2018-01-12T09:55:52Z

Hopefully application cannot run on the browser (it should have had some errors)

deepu105 · 2018-01-12T09:56:33Z

Might be a glitch, restarted the build Thanks & Regards, Deepu

…

On Fri, Jan 12, 2018 at 10:51 AM, Nils Wild ***@***.***> wrote: Failed: Angular could not be found on the page http://localhost:8080/.If this is not an Angular application, you may need to turn off waiting for Angular. any idea what may cause this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6951 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABDlF0EY9pzfnzIgDv_-lcSrT1igFRnyks5tJysdgaJpZM4RUyHh> .

sendilkumarn · 2018-01-12T09:58:32Z

@NilsWild Please be never sorry when contributing to opensource 👍 This is an amazing work 👍

jdubois · 2018-01-12T10:00:14Z

Yes what you did @NilsWild is awesome! Thanks so much.

NilsWild · 2018-01-12T11:14:25Z

Thank you all

@deepu105 failed for same reason

deepu105 · 2018-01-13T15:33:38Z

@mraible need your help here, the ngx-mariadb-oauth2-sass-infinispan build is failing and I tried it locally and seems like protractor tests are failing as the keycloak login screen appears as soon as I launch the application, even before clicking sign in, I guess something changed due to the new Angular Http client used here, probably the way interceptors work. Could you take a look?

mraible · 2018-01-13T15:40:47Z

Do you need me to look asap? I prefer not to work on the weekends, but I'm happy to help too. Can it wait until Tuesday? Monday is a holiday in the US.

…

On Jan 13, 2018, at 08:33, Deepu K Sasidharan ***@***.***> wrote: @mraible need your help here, the ngx-mariadb-oauth2-sass-infinispan build is failing and I tried it locally and seems like protractor tests are failing as the keycloak login screen appears as soon as I launch the application, even before clicking sign in, I guess something changed due to the new Angular Http client used here, probably the way interceptors work. Could you take a look? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

NilsWild · 2018-01-13T15:54:49Z

@deepu105 thanks for the hint i think i got it stupid mistake by me

@mraible

deepu105 · 2018-01-13T16:03:53Z

@mraible no hurry at all and I guess @NilsWild got it. @NilsWild if you are stuck please let Matt know

NilsWild · 2018-01-13T16:46:01Z

build looks good now :D

deepu105 · 2018-01-13T18:34:47Z

@jhipster/developers can somebody take this on a test drive?

kamilcieslak · 2018-01-14T18:35:59Z

while writing tests for app/shared/model package I've noticed, that _request-util.ts:createRequestOption
changed its behavior
previous implementation only took into consideration predefined parameters (page, size, sort, query and filter), leaving alone other passed parameters
new implementation will add all passed properties to output/query parameters

Is that desired behavior?

NilsWild · 2018-01-14T18:50:00Z

Hm... actually i just kept the implementation with some slightly modifications in method parameters (to keep the signature as it is right now) that was discussed in the corresponding issue. As you pass a "req" - the request options as an object - i think it should also be the desired behavior to create httpParams out of those.

Just my opinion atm.

deepu105 · 2018-01-18T17:09:24Z

I'm merging this so we have time to check

deepu105 · 2018-01-18T19:18:13Z

@NilsWild both your PR merged. I also released ng-jhipster 0.4.0, so you can upgrade to that and remove angular/http
Thanks for your huge work

jdubois · 2018-01-18T19:38:37Z

Yes thank you for your awesome work!!

NilsWild · 2018-01-18T20:37:40Z

@deepu105 i create a new pull request for that purpose?

thanks to everyone for the kind words

sendilkumarn · 2018-01-18T20:38:36Z

yeah sure please.

- Use: jhipster/generator-jhipster@26e7ff2 - Migrate to HttpClient: jhipster/generator-jhipster#6951 - Refactor folder: shared -> core

Thisen and others added 15 commits August 31, 2017 20:03

Migrated account and admin module

740ec9e

Merge branch 'master' of https://github.com/jhipster/generator-jhipster

960e785

More migrations and fixes

e1104d4

merged Thisens repository

7c294e9

remove orig files

31a769c

migrated auth-interceptor to @angular/common/http

047dc8d

migrated auth-expired.interceptor to @angular/common/http

041fb50

migrated errorhandler.interceptor to @angular/common/http and updated…

049e012

… alert-error component accordingly

migrated notification.interceptor to @angular/common/http

5a7d16c

removed now unnecessaray http provider

3f59491

forgot to delete the ts file of http provider

542d094

migrated entities to @angular/common/http

073cce3

migrated auth-jwt.service to @angular/common/http

fdcc558

migrated tests to @angular/common/http and remove dependency to old @…

3d330cc

…angular/http

fixed interceptor import conditions

1e92bed

deepu105 reviewed Jan 5, 2018

View reviewed changes

Nils Wild added 2 commits January 5, 2018 21:03

cleanup based on comments

84fcd68

fixed wjitespace

2adcb39

sendilkumarn previously requested changes Jan 6, 2018

View reviewed changes

Nils Wild added 6 commits January 6, 2018 09:59

provide interceptors with class instead of factory method

b9ec610

removed injecto for auth-expired.interceptor

5e1edd7

remove injector form notification.interceptor

ebf9f0a

made user service more consistent in return types

0468bd9

changed test to fit ne HttpResponse

4a35965

fixes

4607784

Nils Wild added 2 commits January 6, 2018 11:13

cleanup unused imports and some other lint errors

efa0c25

fixed last lint errors in tests

bfdbe06

deepu105 changed the base branch from master to spring-boot-2 January 13, 2018 14:23

deepu105 changed the base branch from spring-boot-2 to master January 13, 2018 14:23

fixed misssing negation that i lost when i converted the interceptors

6cbfad8

kamilcieslak mentioned this pull request Jan 14, 2018

Add more tests for Angular code #6581

Closed

deepu105 mentioned this pull request Jan 15, 2018

add prettier and tslint compatibility #6906

Merged

4 tasks

deepu105 merged commit ed69799 into jhipster:master Jan 18, 2018

NilsWild mentioned this pull request Jan 18, 2018

removed unused dependency angular/http and upgraded ng-jhipster to 0.4.0 #7004

Merged

4 tasks

jdubois added this to the 4.14.0 milestone Jan 30, 2018

pascalgrimaud mentioned this pull request Mar 9, 2018

[develop] align to JHipster v5 jhipster/jhipster-registry#251

Merged

4 tasks

		import { PaginationConfig } from './blocks/config/uib-pagination.config';
		import { HTTP_INTERCEPTORS } from '@angular/common/http';


		return this.http.post(SERVER_API_URL + 'api/authentication', data, { headers });
		return this.http.post(SERVER_API_URL + 'api/authentication', data, { headers: headers});

migration to HttpClient #6951

migration to HttpClient #6951

Conversation

NilsWild commented Jan 5, 2018 • edited

deepu105 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

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

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

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

sendilkumarn commented Jan 6, 2018

NilsWild commented Jan 6, 2018

pascalgrimaud commented Jan 6, 2018

deepu105 commented Jan 12, 2018

NilsWild commented Jan 12, 2018

sendilkumarn commented Jan 12, 2018

deepu105 commented Jan 12, 2018 via email

sendilkumarn commented Jan 12, 2018

jdubois commented Jan 12, 2018

NilsWild commented Jan 12, 2018

deepu105 commented Jan 13, 2018

mraible commented Jan 13, 2018 via email

NilsWild commented Jan 13, 2018

deepu105 commented Jan 13, 2018

NilsWild commented Jan 13, 2018

deepu105 commented Jan 13, 2018

kamilcieslak commented Jan 14, 2018

NilsWild commented Jan 14, 2018 • edited

deepu105 commented Jan 18, 2018

deepu105 commented Jan 18, 2018

jdubois commented Jan 18, 2018

NilsWild commented Jan 18, 2018

sendilkumarn commented Jan 18, 2018

NilsWild commented Jan 5, 2018 •

edited

NilsWild commented Jan 14, 2018 •

edited