New delegate API for event listener #149

Toxantron · 2016-04-10T08:06:35Z

So this is what the the event listener API might look like. Feedback welcome.

Toxantron · 2016-04-10T08:07:43Z

I will prepare possible usages of this new api on a branch to make sure I can push commits based on feedback to the master branch.

justinsa · 2016-04-12T05:07:16Z

It is always good to step back for a moment when building a new feature and consider whether it is worth maintaining backwards compatibility. The ideal is that we move the library into an eventing model where extension functions are part of the documentation instead of there being multiple implementations embedded in the core library. A user can then extend through configuration and recipes.

To make the code cleaner, what if the readFromRoute behavior was moved into the documentation as a recipe and the eventListener is then redefined as a function bound to the scope of that, so:

eventListener.bind(that)();

It should probably be renamed to: trackRouteFn instead of listener, since it is not a listener.

Toxantron · 2016-04-12T07:59:40Z

That would mean a little more refactoring. Especially since readFromRoute is not only part of the tracking but also changes the behavior of getUrl(). This would move the library towards a core/plugin design which has defined interfaces for replacing logic. Since I love doing stuff like this it would make the library more complex and my intentions where to maintain current features and only modify the internal structure so we can add different implementations of trackRouteFn(I like this name) as requirements popup.

In addition to that I just wanted to extend this by opening the API to 'advanced users'. The first commit was only to demonstrate new API while maintaining full compatibility and unit test compliance.

Toxantron · 2016-04-12T08:09:38Z

I added support for setting custom version of trackRouteFn to complete the original idea. This is what I had in mind as far as the refactoring goes. Now when something like #145 pops up we can simply extend the block and we're good.

This has neither the flexibility nor the professionalism of your approach but it keeps the library simple and even though the code grows it remains maintainable.

justinsa · 2016-05-28T04:42:44Z

index.js

+
+      // Custom route tracking function
+      this.trackRouteFunction = function(trackingFunction) {
+        trackRouteFn = trackingFunction;


Check that trackingFunction is actually a function before assigning it. No need to throw an error if it is not, just don't set trackeRouteFn.

Toxantron · 2016-05-31T13:42:43Z

Ok, so I replaced nullwith typeof === 'function' and added the sample for hybrid mobile support from #145.

Right now I am trying to think of a nice API to pass _that to trackRouteFn. This way #155 could be resolved by using the new delegate API like:

AnalyticsProvider.trackRouteFunction(function(_that) {
// Some scope call
_that.trackPage();
});

However this won't work because the whole idea was to provide a function that matches the event signature. Any ideas?

Toxantron · 2016-06-02T16:50:51Z

I think I have an idea how to pass in the interal object for expert users.

if (typeof trackRouteFn === 'function') {
  // Redirct event to custom function, passing in protected object
  eventListener = function () {
    var args = Array.prototype.slice.call(arguments);
    var newArgs = [ that ].concat(args);
    trackRouteFn.apply(null, newArgs);
  }
}

justinsa · 2016-06-05T06:25:20Z

index.js

-              // and those explicitly marked as 'do not track'
-              if (!$route.current || !$route.current.templateUrl || $route.current.doNotTrack) {
+        // This section adds different listeners based on the configuration
+        var eventListener;


This should be set to null, for good practice.

justinsa · 2016-06-05T06:36:06Z

@Toxantron why not use binding so the this in the function is that.

if (typeof trackRouteFn === 'function') {
  // Redirect event to custom function, passing in protected object
  eventListener = function () {
    trackRouteFn.apply(that, arguments);
  }
}

Toxantron · 2016-06-06T13:07:33Z

Good point. I thought of binding like trackRouteFn.bind(that) which doesn't work since angular will assign a new this when adding the listener.
However your proposal is shorter and easier to explain to developers.

I feel like this approaches your original proposal. We could even consider moving the other functions to the provider object and passing them back into the setter like so:

this.mobileTracker = function(event, state) {
  // ...
};

// in user code
AnalyticsProvider.trackRouteFunction(AnalyticsProvider.mobileTracker);

I am not sure if it brings any benefit and would unnecessarily break compatibility, but we would have the chance. For now I think we should use the API to cover current requirements and give users to override the behavior if desired. Either in the README or an additional file we could collect FUF (Frequently Used Functions) like the callback from #155. It could also include samples to skip tracking:

AnalyticsProvider.trackRouteFunction(function() {
  if (userCondition)
      this._trackPage();    
});

justinsa · 2016-06-13T05:26:38Z

index.js

              // Avoid tracking undefined routes, routes without template (e.g. redirect routes)
              // and those explicitly marked as 'do not track'
              if (!$route.current || !$route.current.templateUrl || $route.current.doNotTrack) {
-                return;
+                  return;


This got 4 space indentation accidentally.

Man, indentation is a pain in the ass. Will fix ASAP.

justinsa · 2016-06-13T05:29:06Z

@Toxantron this is looking solid. Are there tests for everything? Can you add one for the "screeName" being sent in hybridMobileSupport case? Last ask, I promise.

Toxantron · 2016-06-13T06:27:51Z

Thanks, but I don't think it is ready to merge just yet. We allready have unit tests for most of it like tracking and route reading. I will add some for the customTrackFn feature and maybe test a use case similar to #155. For #145 we will need to add appName to the configuration as pointed out in #161.

Toxantron · 2016-06-14T20:42:23Z

So I fixed indentation and added unit tests. Current state would close #155.

@justinsa where should we add the appName? The account object?

Toxantron · 2016-06-14T21:06:12Z

I had a look at the account object and it didn't fit. I added it to the config and adapted tracking function and unit test.

This fixes #145 and #161.

justinsa · 2016-06-16T02:50:50Z

index.js

@@ -14,6 +14,7 @@
    .provider('Analytics', function () {
      var accounts,
          analyticsJS = true,
+          appName,


Change this to hybridMobileAppName. That makes it clearly related to hybridMobileSupport and not a generic configuration value.

Also, please expose this value in the configuration property of the return object. All variables are exposed there (near the end of the file). This variable also needs to be listed in the README.md as available under the configuration property.

justinsa · 2016-06-17T06:10:14Z

@Toxantron Do you know if it is legal for the appName property to be undefined in the call to GA?

Toxantron · 2016-06-17T06:14:24Z

I see what you are getting at. I think from documentation it stated that
appName is mandatory. I will log a warning and add a unit test.

justinsa · 2016-06-17T06:18:04Z

index.js

+          if (hybridMobileSupport) {
+            // For mobile devices we need to track a screen rather than a page
+            eventListener = function (event, state) {
+              that._send('screenview', {


Log the warning inside the eventListener function, if you weren't already thinking of doing that. Do you think it should still make the _send call?

That's what I had in mind. I haven't decided yet. I will setup a little test locally and see how google analytics handles the missing field. I will also add a test to ensure the values are present in Analytics.configuration. Once it is done we should be good to go.

Toxantron · 2016-06-17T06:35:14Z

Or maybe @josephlaw can answer the question.. He seems to have more experience with the mobile mode.

josephlaw · 2016-06-17T09:08:21Z

My hybrid app temporary use below

   // GA with hybrid app using screenview(app) instead of pageview(web)
    // https://developers.google.com/analytics/devguides/collection/analyticsjs/screens#using_filters_for_app-only_or_web-only_views
    $rootScope.$on('$stateChangeSuccess', function(event, state) {
        Analytics.send('screenview', {
            screenName: state.name,
            appName: 'xxxxx',
            appId: 'X.34567.89',
            appVersion: '0.1',
            appInstallerId: 'apk',
        });
    });

Toxantron · 2016-06-17T10:16:04Z

@josephlaw what would you expect the library to do when appName was not set? Can you tell how google analytics handles it?

Toxantron · 2016-06-17T10:16:37Z

Wrong button.

josephlaw · 2016-06-17T10:30:34Z

If appName not set then the GA do nothing.

Toxantron · 2016-06-17T11:35:36Z

Ok, then we only log exception and not send the call at all. Agreed @justinsa?

justinsa · 2016-06-17T15:00:43Z

We should allow for any arbitrary optional values to be defined as well:

e.g.,

            appId: 'X.34567.89',
            appVersion: '0.1',
            appInstallerId: 'apk',

Otherwise this isn't going to be very useful to @josephlaw since he will need to override the behavior anyways. Perhaps instead of mobileAppName the user can provide mobileAppProperties.

Toxantron · 2016-06-17T17:32:18Z

I agree. And then simply copy properties into the call.

Toxantron · 2016-06-18T08:28:01Z

@justinsa what do you think?

Toxantron · 2016-06-20T08:01:33Z

index.js

+              }
+              // Create a copy for the tracking function to avoid manipulation during transmission
+              var args = angular.copy(mobileAppProperties);
+              args.screenName = state.name;


We may need to differentiate between ngRoute and ui-router here. The sample taken from #145 seems to use ui-router while our default is ngRoute.

Toxantron · 2016-07-14T23:20:09Z

@justinsa how do we proceed here?

justinsa · 2016-07-15T14:20:46Z

@Toxantron This is why we should remove these altogether and instead have a cookbook available that examples specific configurations. Only hybrid mobile cares about this and that should be configured by the developer for their specific scenario. If we simplify all the configuration code we have added and move that to a cookbook then support one reasonable default per handler (like ngRoute), this library becomes better. We can also rev the major version so it is clear the change breaks the past.

Toxantron · 2016-07-15T23:25:17Z

You mean moving just the different delegates or the configuration as a whole? Would you mind adding a small sample of what you have in mind?

Toxantron mentioned this pull request Apr 10, 2016

Mobile app: Is it possible to track screens #145

Open

Toxantron mentioned this pull request Apr 16, 2016

How to use it with ui-router v1.0.x? #153

Open

justinsa reviewed May 28, 2016
View reviewed changes

justinsa reviewed Jun 5, 2016
View reviewed changes

Toxantron mentioned this pull request Jun 7, 2016

add appName into AnalyticsProvider/setAccount #161

Open

justinsa reviewed Jun 13, 2016
View reviewed changes

justinsa reviewed Jun 16, 2016
View reviewed changes

justinsa reviewed Jun 17, 2016
View reviewed changes

Toxantron closed this Jun 17, 2016

Toxantron reopened this Jun 17, 2016

Toxantron reviewed Jun 20, 2016
View reviewed changes

Toxantron mentioned this pull request Nov 2, 2016

Exclude url from tracking with ui-router #178

Closed

Toxantron closed this Jan 12, 2017

Toxantron force-pushed the master branch from 8aaa563 to 92768a5 Compare January 12, 2017 05:48

New delegate API for event listener #149

New delegate API for event listener #149

Conversation

Toxantron commented Apr 10, 2016

Toxantron commented Apr 10, 2016

justinsa commented Apr 12, 2016

Toxantron commented Apr 12, 2016

Toxantron commented Apr 12, 2016

Choose a reason for hiding this comment

Toxantron commented May 31, 2016 • edited Loading

Toxantron commented Jun 2, 2016

Choose a reason for hiding this comment

justinsa commented Jun 5, 2016

Toxantron commented Jun 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsa commented Jun 13, 2016

Toxantron commented Jun 13, 2016

Toxantron commented Jun 14, 2016

Toxantron commented Jun 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Toxantron Jun 16, 2016 via email

Choose a reason for hiding this comment

justinsa commented Jun 17, 2016

Toxantron commented Jun 17, 2016 • edited Loading

Choose a reason for hiding this comment

Toxantron Jun 17, 2016 via email

Choose a reason for hiding this comment

Toxantron commented Jun 17, 2016

josephlaw commented Jun 17, 2016

Toxantron commented Jun 17, 2016

Toxantron commented Jun 17, 2016

josephlaw commented Jun 17, 2016

Toxantron commented Jun 17, 2016

justinsa commented Jun 17, 2016

Toxantron commented Jun 17, 2016 via email

Toxantron commented Jun 18, 2016

Choose a reason for hiding this comment

Toxantron commented Jul 14, 2016

justinsa commented Jul 15, 2016

Toxantron commented Jul 15, 2016

Toxantron commented May 31, 2016 •

edited

Loading

Toxantron commented Jun 6, 2016 •

edited

Loading

Toxantron commented Jun 17, 2016 •

edited

Loading