♻️AMP Analytics: Linker and cookie configs added for WebEngage vendor #26785
Conversation
ayushgupta29
requested review from
zhouyx and
calebcordry
and removed request for
calebcordry
|
@calebcordry Hey, please review this PR. |
|
@ayushgupta29, I think you want @zhouyx or @micajuine-ho, they are the experts :) |
|
@micajuine-ho Can you please take a look at this again? |
| @@ -33,6 +33,22 @@ const WEBENGAGE_CONFIG = jsonLiteral({ | |||
| 'request': 'wePageview', | |||
| }, | |||
| }, | |||
| 'linkers': { | |||
| '_we_linker': { | |||
| 'destinationDomains': ['*'], | |||
There was a problem hiding this comment.
Are you sure you want to use this wildcard to match all destination domains?
There was a problem hiding this comment.
@micajuine-ho Yes. We have clients who have a lot of domains working with a single license. Otherwise if a visitor switch domains, it would again be counted a new user.
There was a problem hiding this comment.
I have the same question here. Decorate every outgoing links could potential pass user id information to domains that the client doesn't own.
It can also lead to issues when your vendor setting is used widely, the destination domain could incorrect digest the incoming linker param even the previous domain never intend to decorate the url.
I understand that some clients want to decorate all destination domains to simplify their configurations. In my opinion, we should first recommend them including their domain list. If destinationDomains: ['*'] is needed, I would avoid making that as default.
Please let us know what do you think. Thank you!
There was a problem hiding this comment.
@zhouyx Yeah, it kinda makes sense. Since the canonical domain and the source domain is by default included. And if some client requires every outgoing domain to be decorated with linker param then anyway we can override it in the configuration on the frontend.
I have removed the destinationDomains param from the configuration.
There was a problem hiding this comment.
Ok, changes LGTM.
Pinging @zhouyx for sanity check before merging.
|
@zhouyx Can you please do a quick sanity check for this PR and merge? |
|
Thank you @ayushgupta29 LGTM. PR Merged |
For vendor WebEngage Linker and Cookie configs have been added in order to make sure that the user journey across differently served AMP pages and Non-AMP is synced.