-
Notifications
You must be signed in to change notification settings - Fork 4
CM-1181 - Send page URL to the identity resolution endpoint #352
Conversation
src/pixel/url-collector.ts
Outdated
@@ -29,6 +29,17 @@ export function collectUrl(state: UrlState): [string, boolean, string[]] { | |||
return [url.toString(), pathRemoved, blockedParams] | |||
} | |||
|
|||
export function stripQueryAndPath(pageUrl?: string): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty string will be stripped by the query builder default. In case this is the desired behaviour, I propose more explicitly communicating that in the types by returning undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, desired behaviour. Changed the signature to stripQueryAndPath(pageUrl: string): string
and the impl uses onNonNull
now.
src/idex.ts
Outdated
@@ -59,6 +62,7 @@ export class IdentityResolver { | |||
.addOptional('gpp_as', nonNullConfig.gppApplicableSections?.join(',')) | |||
.addOptional('cd', nonNullConfig.cookieDomain) | |||
.addOptional('ic', encodeIdCookie(nonNullConfig.resolvedIdCookie), { stripEmpty: false }) | |||
.addOptional('pu', stripQueryAndPath(nonNullConfig.pageUrl)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine, but if you want your helper function to only deal with non nullable types, there is a onNonNull helper you could use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Added that.
Send the page URL to the identity resolution endpoint. This is helpful in cases where the origin header is not available.
Author Todo List:
Ready For Review
status