[FSSDK-8993]feat: switch odp event rest endpoint #808
Conversation
|
Tested in ODP JS Test app on regular Chrome instance - seems to work as expected! Great job! Note: |
| @@ -582,14 +585,29 @@ describe('javascript-sdk (Browser)', function() { | |||
|
|
|||
| const testFsUserId = 'fs_test_user'; | |||
| const testVuid = 'vuid_test_user'; | |||
| var clock; | |||
| const requestParams = new Map; | |||
There was a problem hiding this comment.
Nit: Usage of Map isn't needed in the succeeding code. Can use const requestParams = {} instead, or refactor the following code to make use of Map() by calling the associated functions.
i.e.
const requestParams = new Map();
requestParams.set('endpoint', endpoint);
requestParams.get('endpoint');
More info: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map
There was a problem hiding this comment.
Good catch- Refactored to Map for the ease of teardown via the clear method and since it worked I assumed dot notation was syntactic sugar for the getters/setters. Will fix!
| * @returns True if in the browser | ||
| * @private | ||
| */ | ||
| export function browserMode(): boolean { |
There was a problem hiding this comment.
Nit: Thoughts on renaming to isBrowser() or isBrowserContext()?
| this.queueSize = queueSize || defaultQueueSize; | ||
| this.batchSize = batchSize || DEFAULT_BATCH_SIZE; | ||
| if (flushInterval === 0) { | ||
|
|
||
| if (flushInterval === 0 || isBrowser) { |
There was a problem hiding this comment.
Nit: Thoughts on leaving a comment here and/or in OdpOptions to denote certain odpOptions (queueSize, batchSize, flushInterval) will always be overridden in the browser context?
|
Left some nits - otherwise LGTM overall! |
Summary
Changed the behavior of sendOdpEvent in the browser to utilize the ODP Pixel API (https://jumbe.zaius.com/v2/zaius.gif) via GET call.
Test plan
Ticket
FSSDK-8993