build js

All relevant tests now passing, so this is ready for review with particular consideration to implications for other plugins or unusual tracking setups.

I looked through the code changes and imho they are looking fine. The only "issue" I could see is the slightly changed behavior when using multiple trackers. But that actually should only affect using the visitorid in the trackers before a first tracking request was send. To circumvent this, I guess the only possible solution would be to somehow store the initital visitorid in a global scope, that can be accessed in each tracker instance. But not sure if that is needed.
@tsteur do you think we should move that to a global scope?

@sgiehl I don't quite understand the issue. AFAIK, we expect each tracker to have a different visitorId, not the same. But I might remember it wrongly. In what case exactly would it cause an issue? Like how can it be reproduced?

These tests suggest we want the same visitorId across trackers

They are only the same if they point to the same URL and site.

If there are different trackers used (pointing to a different site, matomo url, or cookie prefix), then they should be different. Which we can also see in

Meaning it shouldn't be global if I see this right.

Because in

So the test is definitely expecting the right thing, the question is why isn't the delayedTracker reading the same visitorId. Maybe it's not returning the correct tracker instance? My random guess might be calling tracker.removeAllAsyncTrackersButFirst() in the beginning might fix the issue in case there's a tracker configured differently from a previous test.

@sgiehl I don't think that be a problem that often (using getVisitorId and manually creating the tracker)? Do you maybe have a specific implementation example where this be a problem? Do I see this right if getVisitorId is called too early it would return an empty visitor ID?

If we set the cookie when getVisitorId is called then we still have the original problem of potentially having cookies set when disabled. getVisitorId wouldn't return an empty visitor ID if called early, it would call loadVisitorIdCookie which will generate a UUID and return a new visitor ID if no cookie exists yet.

That be fine in that case @justinvelluppillai 👍 At least then people would have a way to fix this issue by calling getVisitorId after configuring cookie consent etc. And we improve the situation for 99.8% of the people

build js

build js

build js

build js

Nice one thanks @tsteur. I've implemented that approach here and it passes tests without making those changes 👍

A couple of unrelated tests fail but this is good for review.

@justinvelluppillai there is also getVisitorInfo, which currently might return varying userids until a cookie was set, guess we should set the cookie in that method as well.
Also I'm not sure if it is expected that the visitorId is currently empty until the cookie could be set. But I guess this actually already was the case before.

build js

@sgiehl I've added to now set visitor id cookie in getVisitorInfo also. And yes, this doesn't change behaviour from before with the visitorId before the cookie is set, except previously the visitorId cookie was set immediately if possible.

build js

build js