Access vars in analytics #1748
Access vars in analytics #1748
Conversation
dvoytenko
force-pushed
the
access27
branch
4 times, most recently
from
0216cb7 to
de7116d
Compare
I'll try to do a detailed review tomorrow but the commit looks good overall.
It might be better use an example with custom config. With respect to GA, is subscriber id something that google can associate with a user? If not, you can send the value as custom dimensions in GA. If subscriber id is pii to google, GA can't accept it. Some kind of protection there would be nice. Maybe have a field in analytics config to say allowSensitive: true/false and then pass that into url-replacements?
This is excellent! Thank you! I could not have thought of this and I have been trying to find out how to do this. Again, I'll look at the full code tomorrow but the overall design looks great. One quick question: How do you ensure that when an extension is dependent on another one, everything loads up correctly? |
We have an |
@avimehta @rudygalfi This, today, is a responsibility of the author, however, isn't it? I'm wondering if we just need to say "Use each analytics package as defined in the respective EULA" and that would be sufficient? |
|
@dvoytenko Yes, I think what you said is true. |
|
yes, that would work. |
dvoytenko
force-pushed
the
access27
branch
2 times, most recently
from
ddd3416 to
f814a8e
Compare
dvoytenko
force-pushed
the
access27
branch
9 times, most recently
from
462bb1b to
19fc34c
Compare
|
@cramforce @avimehta @rudygalfi This is now ready for review. PTAL. |
| * @return {*|null} | ||
| */ | ||
| getAccessValue_(getter, expr) { | ||
| return accessServiceFor(this.win_).then(accessService => { |
There was a problem hiding this comment.
Instead make accessServiceForOrNull?
Cid has the same
|
LGTM with a few comments |
| @@ -76,6 +76,28 @@ For instance: | |||
| ``` | |||
| may make a request to something like `https://foo.com/pixel?href=https%3A%2F%2Fpinterest.com%2F`. | |||
|
|
|||
| ### SOURCE_URL | |||
|
|
|||
| Use the special string `SOURCE_URL` to add the source URL of the current document to the URL | |||
There was a problem hiding this comment.
Period at end of sentence.
Closes #1556.
Closes #1747.