Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Angular 4.4 support #1002

Merged
merged 14 commits into from
Oct 6, 2017
Merged

feat: Angular 4.4 support #1002

merged 14 commits into from
Oct 6, 2017

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

@ghost ghost assigned vakrilov Sep 21, 2017
@ghost ghost added the in progress label Sep 21, 2017
@vakrilov
Copy link
Contributor Author

uitests

@@ -6,18 +6,18 @@
"nativescript": {
"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

}

function isLocalRequest(url: string): boolean {
return url.indexOf("~") === 0 || url.indexOf("/") === 0;
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

reject(responseOptions("Not Found", 404, url));
}
}));
}
}

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants