-
Notifications
You must be signed in to change notification settings - Fork 222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extend cross-domain linking with more user/session information #1233
Extend cross-domain linking with more user/session information #1233
Conversation
8c40318
to
34fb605
Compare
BundleMonFiles added (6)
Total files change +98.33KB 0% Final result: ✅ View report in BundleMon website ➡️ |
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.
In terms of the configuration of the feature, there are a couple of things on my mind:
- Nit: Would "optional" be better than "extended" as a name for the extra properties? Extended feels a bit like enhanced ecommerce – what will we call the extended extended version? :D
- I think it would make sense to make the optional properties configurable – I can imagine some people might not want to send the
userId
orappId
in the links but want thesessionId
.
My suggestion would be to make the configuration something like this:
{
crossDomainLinkerOptionalProps: {
userId: true,
sessionId: true,
sourceId: true,
sourcePlatform: true,
reason: true
}
}
We could also let users enable all optional props as you do now:
{
crossDomainLinkerOptionalProps: true
}
About the choise of If though we decide to keep the same formatting (split with dots), optional will be a better naming. |
34fb605
to
7f0f14a
Compare
027700b
to
6183a19
Compare
6183a19
to
616fb37
Compare
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.
Just one nitpick about the dots at the end of the string but otherwise LGTM
616fb37
to
e709c99
Compare
export interface ExtendedCrossDomainLinker { | ||
domainUserId?: string; | ||
/* Timestamp of the cross-domain link click. */ | ||
timestamp?: number; |
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.
Is timestamp
required for this type? It looks like the timestamp
value will always come from here
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 type is only used internally for the creation of the domain values. I moved it around a bit to make it more clear.
35c3112
to
a770521
Compare
a770521
to
3822437
Compare
…tional configuration
3822437
to
97e2ee2
Compare
Extending the cross-domain linking capability with a new parameter format adding the information of cross-navigation to a URLEncoded(B64(contents)) value which will be placed in the cross-domain linker param (
_sp
).The tracker now accepts a
useExtendedCrossDomainLinker
parameter of typeExtendedCrossDomainLinkerOptions
:The default behaviour is not changed.
Notes:
testing-library/dom
allowing us to test DOM interactions on unit type of tests.