New delegate API for event listener

[Toxantron]

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

[Toxantron]

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]

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]

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]

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.

[Toxantron]

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]

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]

@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]

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]

@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]

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]

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

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

[Toxantron]

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]

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

[Toxantron]

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.

[Toxantron]

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

[josephlaw]

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]

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

[Toxantron]

Wrong button.

[josephlaw]

If appName not set then the GA do nothing.

[Toxantron]

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

[justinsa]

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]

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

[Toxantron]

@justinsa what do you think?

[Toxantron]

@justinsa how do we proceed here?

[justinsa]

@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]

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?