[RUM-24816][FEATURE] added experimental triggering mechanism. added a… #85
Conversation
…ctions collectors. added xpath collector. added css path collector. added the ability to configure the triggering mechanism
…iblity to pass context from event.
| * @param {Context} context | ||
| */ | ||
| async prepare (event, context) { | ||
| console.log(context); |
There was a problem hiding this comment.
DoD - Just as a reminder please remove comments/debugging
src/core/Trigger.helper.js
Outdated
| */ | ||
| let context = {}; | ||
|
|
||
| document.querySelector('*').addEventListener('click', async (event) => { |
There was a problem hiding this comment.
@robertdumitrescu INFO - What is the expected behaviour when new elements are rendered to the page - for example, a SPA which re-renders part of the screen? My understanding is that this selector will get stale and any elements that aren't present at this point in time will not trigger actions.
Although propagation can be an issue, attaching to document may be a way to get most of the benefit without having to manage individual handlers?
There was a problem hiding this comment.
@robertdumitrescu PERFORMANCE - How does this stack up performance-wise? We are effectively adding an event listener to every element.- which could be a very broad list of many thousands.
There was a problem hiding this comment.
@robertdumitrescu INFO - This currently attaches to 'click' only. Are there any other gestures we need to attach - I'm thinking about gestures across all scenarios?
src/core/Trigger.helper.js
Outdated
| window.addEventListener('scroll', async (event) => { | ||
| try { | ||
| console.log('Sending event on scroll'); | ||
| await producer.collect(context); |
There was a problem hiding this comment.
@robertdumitrescu MAINTAINABILITY - The event object for context can be a little amorphous and the list of scenarios is likely to grow. This could lead to all sorts of incompatibility and guarding being required further down the chain as we establish exactly what the context was derived from. Perhaps we could establish a tighter interface to avoid inference and guarding?
| @@ -4,11 +4,20 @@ function rciMainAction(tenancyId, rciSdk) { | |||
| const transport = new rciSdk.Transport(targetUrl); | |||
|
|
|||
| // Step 2: Capture your default collectors | |||
| const defaults = rciSdk.collector.defaultCollectors; | |||
| let defaults = rciSdk.collector.defaultCollectors; | |||
| const config = { | |||
There was a problem hiding this comment.
@robertdumitrescu CONSISTENCY - I'm all for composition, leading to maximum flexibility. However, if we're going through the trouble of configuring the collectors, I'd like to see all the concatenation and registering to be handled. For example, if something like the following is provided:
const config = {
transport: new rciSdk.Transport(targetUrl),
onload: {
defaults: true,
enabled: true,
custom: [...]
},
actions: {
defaults: true,
enabled: true,
custom: [...]
}
}
rciSdk.ConfigurationService.run(config);
That should register all triggers and collectors with the relevant producer. Job done!
| actionsBatching: false, | ||
| events: [ | ||
| { | ||
| selector: 'window', eventName: 'load', eventCategory: 'Document', scope: 'state' |
There was a problem hiding this comment.
@robertdumitrescu CONSISTENCY - The naming conventions here are becoming a little unclear - particularly as the names crossover from Web APIs to RCI's Data API. I would suggest referencing terms from the Moz documentation and the Data API directly, for example:
{
target: 'window', type: 'load', rciEventAction: 'full_page_load', rciEventType: 'state'
}
I would also suggest that we plumb the rciEventAction value all the way through to eventAction in the Data API.
| if (context.scope === 'state') { | ||
|
|
||
| /** Stringify actions and attached it to the state beacon */ | ||
| event.eventInfo5 = JSON.stringify(actions); |
There was a problem hiding this comment.
@robertdumitrescu INFO - We need to be careful about string length as anything above 1024 chars will be rejected. This is much lower than I had originally thought... https://docs.real-user-data.eggplant.cloud/open-api/index.html
|
|
||
| /** Append the new action */ | ||
| actions.push(event.eventInfo5[0]); | ||
| if (context.scope === 'state') { |
There was a problem hiding this comment.
@robertdumitrescu MAINTAINABILITY - I'd rather we use ENUMs than lose strings. We have some set up here: https://github.com/TestPlant/real-user-data-sdk-js/blob/development/src/core/constants.js#L1
| * @param {Context} context | ||
| */ | ||
| async prepare (event) { | ||
| event.eventInfo5 = [{}]; |
There was a problem hiding this comment.
@robertdumitrescu KISS - Is the empty object required here..?
|
@robertdumitrescu INFO - In the case where there is only one event (no start or end), where should the data go? |
…ctions collectors. added xpath collector. added css path collector. added the ability to configure the triggering mechanism