snowplow-javascript-tracker icon indicating copy to clipboard operation
snowplow-javascript-tracker copied to clipboard

Deep clone attached global context entites

Open matus-tomlein opened this issue 1 year ago • 3 comments

Since global contexts are "long living" (the tracker keeps a reference to them until they are removed), we should deep clone any objects attached using addGlobalContexts to prevent unexpected changes when the original objects change in user code.

In particular, we should consider deep cloning around these lines.

matus-tomlein avatar Aug 15 '22 11:08 matus-tomlein

Interesting design issue here. There are for sure use cases where users would want it to be static, but I can also think of use cases where one would prefer it to change.

For example, imagine someone is using ping aggregation, and attaching a context for every event which attaches the aggregated data from pings to each event - for example if they want to know what the engaged time was when an interaction happened.

Could we still accommodate both cases?

colmsnowplow avatar Aug 15 '22 11:08 colmsnowplow

That's a good point @colmsnowplow! I didn't think about this and it's true that some users may already be relying on this behaviour of the global context entities that they can change them on the background and the changes will take effect in the events. This could make it a breaking change for them and introduce bugs that could be quite difficult to discover...

However, I think using the context generator callback is a better way to support this use case. In that case, the users set a callback function that generates the context entity on the fly. I think that API is more explicit about how it behaves which is better.

matus-tomlein avatar Aug 15 '22 13:08 matus-tomlein

Ah true, I forgot about those. In that case, I agree that the expected behaviour is as you've originally outlined - I would expect callbacks to be dynamically updated, and vanilla global contexts to be statically defined.

colmsnowplow avatar Aug 15 '22 13:08 colmsnowplow