Skip to content

refactor: call tracer only after isEnabled check #1500

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Sep 7, 2018
Merged

refactor: call tracer only after isEnabled check #1500

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3928845
refactor: call tracer only after isEnabled check
sis0k0 Aug 27, 2018
b405db6
Merge branch 'master' into sis0k0/tracer-is-enabled
sis0k0 Aug 30, 2018
f3a4dea
Merge branch 'master' into sis0k0/tracer-is-enabled
sis0k0 Aug 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 19 additions & 7 deletions nativescript-angular/animations/animation-player.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { KeyframeAnimation }

import { Keyframe, createKeyframeAnimation } from "./utils";
import { NgView } from "../element-registry";
import { animationsLog as traceLog } from "../trace";
import { animationsLog as traceLog, isLogEnabled } from "../trace";

export class NativeScriptAnimationPlayer implements AnimationPlayer {
public parentPlayer: AnimationPlayer = null;
Expand Down Expand Up @@ -41,7 +41,9 @@ export class NativeScriptAnimationPlayer implements AnimationPlayer {
onDestroy(fn: Function): void { this._doneSubscriptions.push(fn); }

play(): void {
traceLog(`NativeScriptAnimationPlayer.play`);
if (isLogEnabled()) {
traceLog(`NativeScriptAnimationPlayer.play`);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@sis0k0 just curious - couldn't this perhaps just be called in tracer itself to avoid having to have these conditionals all over the codebase? ie:

import { isEnabled } from "tns-core-modules/trace";

export function animationsLog(message: string): void {
    if (isEnabled()) {
      write(message, animationsTraceCategory);
   }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The disadvantage of having the check inside the tracer itself is that this will result in two function calls - one for animationsLog and one for isEnabled.

Copy link
Contributor Author

@sis0k0 sis0k0 Sep 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Also if the check was inside the animationsLog function and we invoked animationsLog("something" + random), we would have the overhead of the string concatenation as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for explanation makes sense :) Perhaps there’s a possibility we could also add something to webpack config to strip these all out in production mode as well to coincide with this pr. Ideally with the whole trace enabler control we should probably have ways that devs can strip them when desired as well since keeping core libs slim and performant as possible is always highest goal (which thank you for attention to that here!).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great idea! 10x :)


if (!this.animation) {
return;
Expand All @@ -66,22 +68,28 @@ export class NativeScriptAnimationPlayer implements AnimationPlayer {
}

reset(): void {
traceLog(`NativeScriptAnimationPlayer.reset`);
if (isLogEnabled()) {
traceLog(`NativeScriptAnimationPlayer.reset`);
}

if (this.animation && this.animation.isPlaying) {
this.animation.cancel();
}
}

restart(): void {
traceLog(`NativeScriptAnimationPlayer.restart`);
if (isLogEnabled()) {
traceLog(`NativeScriptAnimationPlayer.restart`);
}

this.reset();
this.play();
}

destroy(): void {
traceLog(`NativeScriptAnimationPlayer.destroy`);
if (isLogEnabled()) {
traceLog(`NativeScriptAnimationPlayer.destroy`);
}
this.onFinish();
}

Expand All @@ -94,13 +102,17 @@ export class NativeScriptAnimationPlayer implements AnimationPlayer {
}

private initKeyframeAnimation(keyframes: Keyframe[], duration: number, delay: number, easing: string) {
traceLog(`NativeScriptAnimationPlayer.initKeyframeAnimation`);
if (isLogEnabled()) {
traceLog(`NativeScriptAnimationPlayer.initKeyframeAnimation`);
}

this.animation = createKeyframeAnimation(keyframes, duration, delay, easing);
}

private onFinish() {
traceLog(`NativeScriptAnimationPlayer.onFinish`);
if (isLogEnabled()) {
traceLog(`NativeScriptAnimationPlayer.onFinish`);
}

if (this._finished) {
return;
Expand Down
42 changes: 31 additions & 11 deletions nativescript-angular/directives/list-view-comp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import { ObservableArray } from "tns-core-modules/data/observable-array";
import { profile } from "tns-core-modules/profiling";

import { getSingleViewRecursive } from "../element-registry";
import { listViewLog, listViewError } from "../trace";
import { listViewLog, listViewError, isLogEnabled } from "../trace";

const NG_VIEW = "_ngViewRef";

Expand Down Expand Up @@ -101,7 +101,9 @@ export class ListViewComponent implements DoCheck, OnDestroy, AfterContentInit {
}

ngAfterContentInit() {
listViewLog("ListView.ngAfterContentInit()");
if (isLogEnabled()) {
listViewLog("ListView.ngAfterContentInit()");
}
this.setItemTemplates();
}

Expand All @@ -115,7 +117,9 @@ export class ListViewComponent implements DoCheck, OnDestroy, AfterContentInit {
this.itemTemplate = this.itemTemplateQuery;

if (this._templateMap) {
listViewLog("Setting templates");
if (isLogEnabled()) {
listViewLog("Setting templates");
}

const templates: KeyedTemplate[] = [];
this._templateMap.forEach(value => {
Expand All @@ -126,15 +130,19 @@ export class ListViewComponent implements DoCheck, OnDestroy, AfterContentInit {
}

public registerTemplate(key: string, template: TemplateRef<ListItemContext>) {
listViewLog("registerTemplate for key: " + key);
if (isLogEnabled()) {
listViewLog(`registerTemplate for key: ${key}`);
}
if (!this._templateMap) {
this._templateMap = new Map<string, KeyedTemplate>();
}

const keyedTemplate = {
key,
createView: () => {
listViewLog("registerTemplate for key: " + key);
if (isLogEnabled()) {
listViewLog(`registerTemplate for key: ${key}`);
}

const viewRef = this.loader.createEmbeddedView(template, new ListItemContext(), 0);
const resultView = getItemViewRoot(viewRef);
Expand All @@ -159,7 +167,9 @@ export class ListViewComponent implements DoCheck, OnDestroy, AfterContentInit {
let viewRef: EmbeddedViewRef<ListItemContext>;

if (args.view) {
listViewLog("onItemLoading: " + index + " - Reusing existing view");
if (isLogEnabled()) {
listViewLog(`onItemLoading: ${index} - Reusing existing view`);
}
viewRef = args.view[NG_VIEW];
// Getting angular view from original element (in cases when ProxyViewContainer
// is used NativeScript internally wraps it in a StackLayout)
Expand All @@ -168,12 +178,16 @@ export class ListViewComponent implements DoCheck, OnDestroy, AfterContentInit {
}

if (!viewRef) {
listViewError("ViewReference not found for item " + index + ". View recycling is not working");
if (isLogEnabled()) {
listViewError(`ViewReference not found for item ${index}. View recycling is not working`);
}
}
}

if (!viewRef) {
listViewLog("onItemLoading: " + index + " - Creating view from template");
if (isLogEnabled()) {
listViewLog(`onItemLoading: ${index} - Creating view from template`);
}
viewRef = this.loader.createEmbeddedView(this.itemTemplate, new ListItemContext(), 0);
args.view = getItemViewRoot(viewRef);
args.view[NG_VIEW] = viewRef;
Expand All @@ -197,17 +211,23 @@ export class ListViewComponent implements DoCheck, OnDestroy, AfterContentInit {

@profile
private detectChangesOnChild(viewRef: EmbeddedViewRef<ListItemContext>, index: number) {
listViewLog("Manually detect changes in child: " + index);
if (isLogEnabled()) {
listViewLog(`Manually detect changes in child: ${index}`);
}
viewRef.markForCheck();
viewRef.detectChanges();
}

ngDoCheck() {
if (this._differ) {
listViewLog("ngDoCheck() - execute differ");
if (isLogEnabled()) {
listViewLog("ngDoCheck() - execute differ");
}
const changes = this._differ.diff(this._items);
if (changes) {
listViewLog("ngDoCheck() - refresh");
if (isLogEnabled()) {
listViewLog("ngDoCheck() - refresh");
}
this.listView.refresh();
}
}
Expand Down
6 changes: 4 additions & 2 deletions nativescript-angular/directives/tab-view.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { TabView, TabViewItem } from "tns-core-modules/ui/tab-view";
import { TextTransform } from "tns-core-modules/ui/text-base";

import { InvisibleNode } from "../element-registry";
import { rendererLog } from "../trace";
import { rendererLog, isLogEnabled } from "../trace";
import { isBlank } from "../lang-facade";

export interface TabViewItemDef {
Expand Down Expand Up @@ -46,7 +46,9 @@ export class TabViewDirective implements AfterViewInit {

ngAfterViewInit() {
this.viewInitialized = true;
rendererLog("this._selectedIndex: " + this._selectedIndex);
if (isLogEnabled()) {
rendererLog("this._selectedIndex: " + this._selectedIndex);
}
if (!isBlank(this._selectedIndex)) {
this.tabView.selectedIndex = this._selectedIndex;
}
Expand Down
12 changes: 8 additions & 4 deletions nativescript-angular/dom-adapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* tslint:disable */
import { Type } from "@angular/core";
import { ɵDomAdapter } from "@angular/platform-browser";
import { rendererLog } from "./trace";
import { rendererLog, isLogEnabled } from "./trace";

export class NativeScriptDomAdapter implements ɵDomAdapter {
static makeCurrent() {
Expand All @@ -12,12 +12,16 @@ export class NativeScriptDomAdapter implements ɵDomAdapter {
const privateAPI = global.require("@angular/platform-browser");
const setRootDomAdapter = privateAPI.ɵsetRootDomAdapter;

rendererLog("Setting root DOM adapter...");
if (isLogEnabled()) {
rendererLog("Setting root DOM adapter...");
}
setRootDomAdapter(new NativeScriptDomAdapter());
} catch (e) {
rendererLog("@angular/platform-browser package not present. NOT setting root DOM adapter...");
if (isLogEnabled()) {
rendererLog("@angular/platform-browser package not present. NOT setting root DOM adapter...");
}
}
}
}
}

hasProperty(_element: any, _name: string) {
Expand Down
62 changes: 46 additions & 16 deletions nativescript-angular/platform-common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import {
} from "@angular/core";
import { DOCUMENT } from "@angular/common";

import { bootstrapLog, bootstrapLogError } from "./trace";
import { bootstrapLog, bootstrapLogError, isLogEnabled } from "./trace";
import { defaultPageFactoryProvider, setRootPage, PageFactory, PAGE_FACTORY } from "./platform-providers";
import { AppHostView } from "./app-host-view";

Expand Down Expand Up @@ -155,18 +155,24 @@ export class NativeScriptPlatformRef extends PlatformRef {
setRootPage(<any>tempAppHostView);
}

bootstrapLog("NativeScriptPlatform bootstrap started.");
if (isLogEnabled()) {
bootstrapLog("NativeScriptPlatform bootstrap started.");
}
const launchCallback = profile(
"nativescript-angular/platform-common.launchCallback",
(args: LaunchEventData) => {
bootstrapLog("Application launch event fired");
if (isLogEnabled()) {
bootstrapLog("Application launch event fired");
}

let bootstrapPromiseCompleted = false;
this._bootstrapper().then(
moduleRef => {
bootstrapPromiseCompleted = true;

bootstrapLog(`Angular bootstrap bootstrap done. uptime: ${uptime()}`);
if (isLogEnabled()) {
bootstrapLog(`Angular bootstrap bootstrap done. uptime: ${uptime()}`);
}

if (!autoCreateFrame) {
rootContent = tempAppHostView.content;
Expand All @@ -178,20 +184,30 @@ export class NativeScriptPlatformRef extends PlatformRef {
bootstrapPromiseCompleted = true;

const errorMessage = err.message + "\n\n" + err.stack;
bootstrapLogError("ERROR BOOTSTRAPPING ANGULAR");
bootstrapLogError(errorMessage);
if (isLogEnabled()) {
bootstrapLogError("ERROR BOOTSTRAPPING ANGULAR");
}
if (isLogEnabled()) {
bootstrapLogError(errorMessage);
}

rootContent = this.createErrorUI(errorMessage);
}
);

bootstrapLog("bootstrapAction called, draining micro tasks queue. Root: " + rootContent);
if (isLogEnabled()) {
bootstrapLog("bootstrapAction called, draining micro tasks queue. Root: " + rootContent);
}
(<any>global).Zone.drainMicroTaskQueue();
bootstrapLog("bootstrapAction called, draining micro tasks queue finished! Root: " + rootContent);
if (isLogEnabled()) {
bootstrapLog("bootstrapAction called, draining micro tasks queue finished! Root: " + rootContent);
}

if (!bootstrapPromiseCompleted) {
const errorMessage = "Bootstrap promise didn't resolve";
bootstrapLogError(errorMessage);
if (isLogEnabled()) {
bootstrapLogError(errorMessage);
}
rootContent = this.createErrorUI(errorMessage);
}

Expand All @@ -205,7 +221,9 @@ export class NativeScriptPlatformRef extends PlatformRef {

@profile
public _livesync() {
bootstrapLog("Angular livesync started.");
if (isLogEnabled()) {
bootstrapLog("Angular livesync started.");
}
onBeforeLivesync.next(lastBootstrappedModule ? lastBootstrappedModule.get() : null);

const autoCreateFrame = !!this.appOptions.createFrameOnBootstrap;
Expand All @@ -226,7 +244,9 @@ export class NativeScriptPlatformRef extends PlatformRef {
this._bootstrapper().then(
moduleRef => {
bootstrapPromiseCompleted = true;
bootstrapLog("Angular livesync done.");
if (isLogEnabled()) {
bootstrapLog("Angular livesync done.");
}
onAfterLivesync.next({ moduleRef });

if (!autoCreateFrame) {
Expand All @@ -237,23 +257,33 @@ export class NativeScriptPlatformRef extends PlatformRef {
},
error => {
bootstrapPromiseCompleted = true;
bootstrapLogError("ERROR LIVESYNC BOOTSTRAPPING ANGULAR");
if (isLogEnabled()) {
bootstrapLogError("ERROR LIVESYNC BOOTSTRAPPING ANGULAR");
}
const errorMessage = error.message + "\n\n" + error.stack;
bootstrapLogError(errorMessage);
if (isLogEnabled()) {
bootstrapLogError(errorMessage);
}

rootContent = this.createErrorUI(errorMessage);

onAfterLivesync.next({ error });
}
);

bootstrapLog("livesync bootstrapAction called, draining micro tasks queue. Root: " + rootContent);
if (isLogEnabled()) {
bootstrapLog("livesync bootstrapAction called, draining micro tasks queue. Root: " + rootContent);
}
(<any>global).Zone.drainMicroTaskQueue();
bootstrapLog("livesync bootstrapAction called, draining micro tasks queue finished! Root: " + rootContent);
if (isLogEnabled()) {
bootstrapLog("livesync bootstrapAction called, draining micro tasks queue finished! Root: " + rootContent);
}

if (!bootstrapPromiseCompleted) {
const result = "Livesync bootstrap promise didn't resolve";
bootstrapLogError(result);
if (isLogEnabled()) {
bootstrapLogError(result);
}
rootContent = this.createErrorUI(result);

onAfterLivesync.next({ error: new Error(result) });
Expand Down
Loading