Skip to content

Conversation

vakrilov
Copy link
Contributor

@vakrilov vakrilov commented Sep 21, 2017

In this PR:

  • Bumped dependencies to angular@4.4.x
  • Implemented NsHttpBackEnd that handles local requests made with HttpClient service
  • Minor 4.4 updates

Resolves #966
Resolves #1001

@vakrilov vakrilov requested a review from sis0k0 September 21, 2017 13:00
@ghost ghost assigned vakrilov Sep 21, 2017
@ghost ghost added the in progress label Sep 21, 2017
@vakrilov
Copy link
Contributor Author

uitests

"id": "org.nativescript.renderer",
"tns-android": {
"version": "next"
"version": "3.2.0-2017-9-12-1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope

return url.indexOf("~") === 0 || url.indexOf("/") === 0;
}

function normalizeLocalUrl(url: string): string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method can be reused in NsHttpBackEnd

}
}

function isLocalRequest(url: string): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is duplicated in NsHttpBackEnd. May be a good idea to extract it in a common module?

return url;
}

private handleLocalRequest(url: string): Observable<HttpEvent<any>> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be the same as _requestLocalUrl from NSHttp except the additional error handling for failed parsing. Can we move the method to a common module (with the error handling)?

const json = JSON.parse(data);
observer.next(createSuccessResponse(url, json, 200));
observer.complete();
} catch (error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catch only SyntaxErrors?

import { NSFileSystem } from "../file-system/ns-file-system";
import { NsHttpBackEnd } from "./ns-http-backend";

export { NsHttpBackEnd } from "./ns-http-backend";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is that reexported from here?

HttpClient, HTTP_INTERCEPTORS, HttpEventType, HttpErrorResponse,
HttpEvent, HttpInterceptor, HttpHandler, HttpRequest
} from "@angular/common/http";
import { Observable } from "rxjs/observable";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from "rxjs/observable" -> from "rxjs/Observable"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch

@SvetoslavTsenov
Copy link
Contributor

e2e

@vakrilov
Copy link
Contributor Author

vakrilov commented Oct 3, 2017

e2e

Copy link
Contributor

@sis0k0 sis0k0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove e2e/router/.vscode dir. Rebasing on top of master may resolve that.

return result;
}

private handleLocalRequest(url: string): Observable<HttpEvent<any>> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of the method is a bit confusing as it matches the name of the imported function.


function normalizeLocalUrl(url: string): string {
return url.replace("~", "").replace("/", "");
private handleLocalRequest(url: string): Observable<Response> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

@vchimev
Copy link
Contributor

vchimev commented Oct 6, 2017

renderer-android

@vakrilov vakrilov merged commit c264453 into master Oct 6, 2017
@NativeScript NativeScript deleted a comment from vakrilov Oct 6, 2017
@jogboms
Copy link

jogboms commented Oct 11, 2017

@vakrilov

ERROR Error: Uncaught (in promise): Error: com.tns.NativeScriptException: Failed to find module: "nativescript-angular/value-accessors/base-value-accessor", relative to: app/tns_modules/

Just installed @next

@angular/*: "^4.4.1"

Narrowed it down to nativescript-drop-down.

I believe we would get some breaking change all-round. Reverting now until a list is ready. 😄

@jogboms
Copy link

jogboms commented Oct 11, 2017

I can see nativescript-angular/value-accessors has been moved to nativescript-angular/forms/value-accessors

@jogboms
Copy link

jogboms commented Oct 12, 2017

nativescript-angular/common/utils no longer exists as well

@vakrilov vakrilov deleted the ng44 branch October 12, 2017 08:47
@vakrilov
Copy link
Contributor Author

Hey @jogboms, thanks for reporting that!
Actually, the nativescript-angular/common/utils was removed a few releases back (before 4.x).
We heave moved the value-accessors inside the forms in this PR(#1003) which is a breaking change for plugins/apps that import it. We are currently working on a patch with a fix.

@jogboms
Copy link

jogboms commented Oct 12, 2017

Okay. Didn't notice that. Just sent a PR over. Quick question tho, does v4.4 require the @next version tns-core-modules

@vchimev
Copy link
Contributor

vchimev commented Oct 12, 2017

Hey @jogboms,
nativescript-angular@4.4 does not require tns-core-modules@next, however, it works with both latest and next versions.

@jogboms
Copy link

jogboms commented Oct 12, 2017

Very well @vchimev. Thanks.

@vchimev
Copy link
Contributor

vchimev commented Oct 13, 2017

nativescript-angular@4.4.1 containing a compatibility fix has been released - here is the changelog.

@jogboms
Copy link

jogboms commented Oct 13, 2017

Nice one. 👍 @vchimev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

6 participants