search

angulartics / angulartics-google-analytics

Google Analytics plugin for Angulartics
MIT License
133 stars 86 forks source link

Improve custom dimension/metrics handling #42

Closed IdanCo closed 8 years ago

IdanCo commented 8 years ago
  1. Allow unsetting of dimensions, for example {dimension1: null}
  2. Improve performance by avoiding long iterations

Detailed explanation in #41

DanWilkerson commented 8 years ago

Thank you for contributing! We do have a PR (#38) that addresses the unsetting issue you raised in #41 that is being checked right now. It separates dimension setting into user and hit levels, which should solve that issue. I like the idea of switching from a truthy check to an undefined check, though, since falsey values may be unintentionally excluded.

I like the idea of trying to make the checking more efficient, but I'm concerned that using a for-in would result in checking non-dimension properties, whereas the for loop currently in place restricts the checking to only dimension and metrics. I'm not sure this is the approach we want to take to make this check more efficient, however; could you make your case for why you're using a for-in and lastIndexOf? Also, if you submit a PR that just switches to checking for truthy/falsey in the for loop, I could merge that. Thanks again for submitting this PR!

DanWilkerson commented 8 years ago

Actually, just realized that #38 also addresses the truthiness issue, which will allow you to set properties to null, false, etc, so we may not need an additional PR. Can you take a look at #38 and let me know if that solves the problems you've raised here and in #41?

IdanCo commented 8 years ago

Excellent! I didn't notice the previous PR, looks like it solves the issue just as well (if not better) than what i suggested. Great job!

Regarding efficiency, i didn't do a benchmark test or anything, it just sounds reasonable that iterating no less than 200 times, is less efficient than just going over exisiting properties. But it's not a deal breaker, I suggest you'll put a pin on it and maybe address it after you'll merge #38 .

DanWilkerson commented 8 years ago

Good point; it does seem pretty unlikely that we'll hit more than 200 properties, and checking 400 values seems a bit excessive. We've merged in #38, so if you want to take a crack at that now, please do :) I'm going to close this in the meantime, if that's okay!