-
Notifications
You must be signed in to change notification settings - Fork 254
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
[HEAP] update browser destination code to load heap js v5 script #2665
base: main
Are you sure you want to change the base?
Conversation
@@ -58,9 +60,16 @@ export const destination: BrowserDestinationDefinition<Settings, HeapApi> = { | |||
required: false | |||
}, | |||
trackingServer: { | |||
label: 'Tracking Server', | |||
label: 'Tracking Server (deprecated)', |
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.
hi @cpsoinos looks like you have labeled this as deprecated. Is it still referenced anywhere in code though? i.e is it actually used at all?
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.
Hi @joe-ayoub-segment, I updated my changes so that this setting is still used, though it is still deprecated. Customers that wish to self-host will have set both trackingServer
and hostname
. When both of these settings are set, Segment will load the Classic SDK (retaining current functionality).
type: 'string', | ||
required: false | ||
}, | ||
ingestServer: { |
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.
Note: adding a new 'optional' setting is OK.
hi @cpsoinos thanks for the PR. Couple questions:
|
Hi @joe-ayoub-segment, thanks for the review. To address your questions:
FYI, I will be OOO all of next week, so if additional changes are required I will address them when I return the week of Jan 13. Thanks in advance! |
Summary
Update the Heap browser destination to use v5 of Heap's javascript snippet.
The Problem
Customers are running into issues with TikTok. We've seen that TT browser web views will strip duplicate key/value pairs on
GET
requests as an "optimization". Our request schema uses an arbitrary key for custom properties. For example, we'll have the query params:k=name1&k=false&k=name2&k=false&k=name3&k=true
, but because of the way TT "optimizes", it will intercept the request before firing off and remove any duplicate query param values, leading to the request being fired having:k=name1&k=false&k=name2&k=name3&k=true
, which ends up causing mismatched k/v pairing on the custom properties when we parse the request on the server-side.The Solution
By upgrading to v5, we replace GET requests with POST so this fixes the issue.
Visuals
Loading the Classic SDK
heap-${appid}.js
is loadedtrack
calls result in a GET request to<host>/h?<params>
identify
calls result in a GET request to<host>/api/identify_v3?<params>
track
andidentify
calls may also cause a GET request to<host>/api/add_user_properties_v3?<params>
Loading the Latest SDK
/config/${appid}/heap_config.js
is loaded/v5/heapjs-static/5.2.6/core/heap.js
is subsequently loaded, using the version retrieved withinheap_config.js
track
calls result in a POST request to/api/capture/v2/track
with attributes in the payloadidentify
calls result in a POST request to/api/capture/v2/identify
with attributes in the payloadtrack
andidentify
calls may also cause a POST request to/api/capture/v2/add_user_properties
with additional attributesTesting
Include any additional information about the testing you have completed to
ensure your changes behave as expected. For a speedy review, please check
any of the tasks you completed below during your testing.