Google Analytics helper #37

bursauxa · 2017-01-04T16:27:47Z

Added analytics service in /helpers. This services supports basic page tracking (automatically triggered on view change, so the function never has to be called manually) and event tracking by calling the provided function.

…e tracking (automatically triggered on view change, so the method never has to be called manually) and event tracking.

sinedied

I like the idea of this PR and its simplicity, thanks for the proposal 👍

Even though more complete libraries like angulartics, having the possibility to quickly add GA page/event tracking through a simple single file service.

After the PR is fixed, I'll merge it to separate feature branch before going to master so it can be enabled or not via a Yeoman prompt, and directly enter the GA account ID from there.

sinedied · 2017-01-05T08:33:04Z

generators/app/templates/sources/main/_main.run.ts

+  // and then it's fired a second time after is has been linked. This is "by design" :-/
+  // (http://stackoverflow.com/questions/31000417/angular-js-viewcontentloaded-loading-twice-on-initial-homepage-load)
+  let loadedOnce = false;
+  vm.$on('$viewContentLoaded', function () {


Why not use $stateChangeSuccess like the hook to update title on route changes?
This should fix the need for the "hack" :)

As per ui-router documentation (link) :

State change events are deprecated, DISABLED and replaced by Transition Hooks as of version 1.0

Note that the view load events are not deprecated. So maybe the real solution is to look into transition hooks ? But that's a different beast altogether.

LOL, missed that since we are still on the 0.3.x version and the 1.x version has not been finalized yet 😅

I have created a separate issue for the migration to using $transitions hooks (see #38 ), for the meanwhile it's still a valid change (until 1.0.0 version is out) that will get rid of the ugly hack, and it will simplify the migration task, as it will be needed anyway as we use it elsewhere.
Thanks for pointing that!

sinedied · 2017-01-05T09:42:46Z

generators/app/templates/sources/main/helpers/analytics/analytics.service.ts

+
+  private init(): void {
+    if (this.config.analyticsAccount !== null) {
+      let analyticsScriptUrl = '//www.google-analytics.com/analytics.js';


would be better to define it as a const on the beginning of the file

sinedied · 2017-01-05T09:43:12Z

generators/app/templates/sources/main/helpers/analytics/analytics.service.ts

+  private init(): void {
+    if (this.config.analyticsAccount !== null) {
+      let analyticsScriptUrl = '//www.google-analytics.com/analytics.js';
+      this.createGoogleAnalyticsObject(window, document, 'script', analyticsScriptUrl, 'googleAnalytics');


Should use this.$window instead of window

Last parameter should be 'ga' instead of 'googleAnalytics' so other libraries depending on google analytics can also use it

Both done (although replacing an explicit field name with an obfuscated one does not sit right with me).

sinedied · 2017-01-05T09:45:09Z

generators/app/templates/sources/main/helpers/analytics/analytics.service.ts

+              private config: IApplicationConfig,
+              logger: LoggerService) {
+
+    this.logger = logger.getLogger('analyticsService');


Logger not needed if it's not used, or maybe it could be used to log tracked events?

Absolutely, logging tracked events was the point. I must have lost it somewhere along the road ; will fix it now.

Done earlier (with previous changes).

sinedied · 2017-01-05T09:59:44Z

generators/app/templates/sources/main/helpers/analytics/analytics.service.ts

+
+    this.logger = logger.getLogger('analyticsService');
+
+    this.init();


I would prefer a public init() method not called in constructor but in app init (in main.run.ts) explicitely, so we can also pass custom GA configuration as an optional argument for the create call.

This is also mandatory for mobile/Cordova compatibility, as specific configuration is needed for GA to work with file:// protocol used in Cordova.

This would also allow us to disable analytics in debug environment, as it is most of the time only wanted on production and it can mess with unit tests by generating extra HTTP calls.

See this example except from a mobile of mine, to make it work with Cordova (located in init() method of main.run.ts, it uses amgulartics but the idea is the same:

// Setup analytics if (!config.environment.debug && $window['ga']) { // Warning, breaks unit tests if included in debug! $window['ga']('create', config.googleAnalyticsId, { 'storage': 'none', 'clientId': $cordovaDevice.getUUID() }); // Allow file:// protocol for cordova $window['ga']('set', 'checkProtocolTask', (data: any) => { data.set('location', 'https://htf2016.app/'); }); $window['ga']('set', 'appVersion', config.version); $analytics.eventTrack('App started', { value: vm.festival.version }); }

I believe the responsibility of customizing the creation of the GA object lies within the analytics service. All the Cordova specific code can be added in analytics-service instead of main-run.

Regarding disabling of analytics in debug, there are some cases where having them on is useful (typically when one wants to test out a newly-added event tracker). Of course you would want a different analytics account for debugging and for actual production, but that is just a matter of setting up the gulp-replace task around IApplicationConfig.googleAnalyticsId. If you want to disable them entirely in debug mode, it can also be done the same way, by setting the value to null in debug mode.

I believe the responsibility of customizing the creation of the GA object lies within the analytics service. All the Cordova specific code can be added in analytics-service instead of main-run.

I completely agree with that, I just posted it to show a mobile setup ;)

Regarding disabling of analytics in debug, there are some cases where having them on is useful (typically when one wants to test out a newly-added event tracker). Of course you would want a different analytics account for debugging and for actual production, but that is just a matter of setting up the gulp-replace task around IApplicationConfig.googleAnalyticsId. If you want to disable them entirely in debug mode, it can also be done the same way, by setting the value to null in debug mode.

In my idea disabling analytics in debug would be done in main.run.ts by calling or not the analyticsService.init(), so if needed the behavior can be changed. But it's a good idea to just check if googleAnalyticsId is defined to activate or not GA in the init() method, so the you just need to move the googleAnalyticsId in the environment object so there can be an value for each environment.

Done: googleAnalyticsId moved from Config to Environment.

sinedied · 2017-01-05T10:03:23Z

generators/app/templates/sources/main/helpers/analytics/analytics.service.ts

+  }
+}
+
+app.service('analyticsService', AnalyticsService);


Since it is a service, unit tests are mandatory (though they will be quite simple as GA will be mocked) and should be added :)

sinedied · 2017-01-05T10:04:14Z

generators/app/templates/sources/main/_main.constants.ts

  version: string;
  environment: IApplicationEnvironment;
  supportedLanguages: Array<string>;
+  analyticsAccount: string;


Would be more explicit to rename it googleAnayticsId

…r-pro Conflicts: generators/app/templates/sources/main/_main.constants.ts

sinedied · 2017-01-06T13:27:31Z

Do not forget the unit tests, so I can merge it ;)

bursauxa · 2017-01-06T14:04:24Z

I did not forget them, I just don't see how to proceed, considering than I can not actually mock GA (which is loaded from the Google URL by a private function). I will come back to this issue when I have more time on my hands.

sinedied · 2017-01-13T18:04:56Z

This PR has been merged on feature/analytics so I can add the generation prompt to opt in/out the service.

bursauxa added 5 commits January 4, 2017 17:23

Added analytics service in /helpers. This services supports basic pag…

15531a4

…e tracking (automatically triggered on view change, so the method never has to be called manually) and event tracking.

Fixed some Yeoman and Typescript issues.

324a620

More Typescript magic because my window is better than their window.

6832800

Added missing type import. Build should be all good now.

555fe01

Added missing semicolon.

d06d2f6

sinedied requested changes Jan 5, 2017

View reviewed changes

bursauxa added 4 commits January 5, 2017 14:12

Made some changes according to code review from pull request.

4b8be83

Made some changes according to code review from pull request.

71d0935

Merge branch 'master' of https://github.com/bursauxa/generator-angula…

d4dc9cc

…r-pro Conflicts: generators/app/templates/sources/main/_main.constants.ts

Moved analytics ID from Config to Environment.

5c84ec4

sinedied changed the base branch from master to feature/analytics January 13, 2017 18:02

sinedied approved these changes Jan 13, 2017

View reviewed changes

sinedied merged commit 4de8401 into angular-starter-kit:feature/analytics Jan 13, 2017


		this.logger = logger.getLogger('analyticsService');

		this.init();

Google Analytics helper #37

Google Analytics helper #37

Uh oh!

Conversation

bursauxa commented Jan 4, 2017

Uh oh!

sinedied left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bursauxa Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sinedied commented Jan 6, 2017

Uh oh!

bursauxa commented Jan 6, 2017

Uh oh!

sinedied commented Jan 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bursauxa Jan 5, 2017 •

edited

Loading