-
Notifications
You must be signed in to change notification settings - Fork 220
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
Deep clone attached global context entites #1086
Comments
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? |
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. |
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. |
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.
The text was updated successfully, but these errors were encountered: