https://github.com/matomo-org/matomo/pull/15970 - Mouve

Site d'origine

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

JS Offline tracking #15970

Merged
merged 6 commits into from Oct 2, 2020
Merged

JS Offline tracking #15970

merged 6 commits into from Oct 2, 2020

Conversation

@tsteur
Copy link
Member

@tsteur tsteur commented May 22, 2020

refs #9939

early stage... usage:

within a service worker do

self.importScripts('https://your.matomo.domain/offline-service-worker.js');
matomoAnalytics.initialize();

to register service worker:

if ('serviceWorker' in navigator) {
  window.addEventListener('load', function() {
    navigator.serviceWorker.register('/your-service-worker.js');
  });
}

We discussed to merge this early in the beta even though it might not fully work yet. This way people can test it easily and give us feedback and review etc.

@tsteur tsteur added this to the 4.0.0 milestone May 22, 2020
@tsteur tsteur modified the milestones: 4.0.0, 4.0.0 RC Jul 28, 2020
tsteur added 3 commits Sep 29, 2020
@tsteur tsteur changed the title JS Offline tracking [WIP] JS Offline tracking Sep 29, 2020
offline-service-worker.js Outdated Show resolved Hide resolved
Copy link
Member

@diosmosis diosmosis left a comment

left one comment on a possible issue.

approach is basically just:

  • if not online, queue requests in indexeddb w/ cdo set to time between now & value creation time
  • when online, play all queued requests
  • in tracker, if cdo is present apply it to timestamp before processing the request

is this correct?

and also it caches piwik.js/matomo.js? is this needed or can it just be cached via an http header?

return response;
});
});
})

This comment has been minimized.

@diosmosis

diosmosis Oct 1, 2020
Member

note: not super important in terms of review, but you can make this a bit more readable via promise chaining:

caches.open('matomo').then(function (cache) {
  return cache.match(event.request);
}).then(function (response) {
  return response || fetch(event.request);
}).then(function (response) {
  cache.put(event.request, response.clone());
  return response;
});

This comment has been minimized.

@tsteur

tsteur Oct 1, 2020
Author Member

👍 makes sense. We can tweak code later once we get feedback the overall idea actually works

offline-service-worker.js Outdated Show resolved Hide resolved
offline-service-worker.js Show resolved Hide resolved
@@ -498,6 +504,10 @@ protected function getCustomTimestamp()
$cdt = strtotime($cdt);
}

if (!empty($cdo)) {
$cdt = $cdt - abs($cdo);
}

This comment has been minimized.

@diosmosis

diosmosis Oct 1, 2020
Member

if a request has a cdt, then should we still apply cdo? ie, if for some reason, the request has cdt=date(2018-12-01 03:04:05)&cdo=30, then shouldn't the given cdt be used anyway? Since it's hardcoded.

This comment has been minimized.

@tsteur

tsteur Oct 1, 2020
Author Member

Initially, when cdt and cdo was used I only used the cdt but then changed it in one of the recent commits as it might not be expected and only adds a benefit / more flexibility to support using both. what would happen in this case is that it removes 30 seconds from the set cdt.

eg 2018-12-01 03:04:05 - 30s = 2018-12-01 03:03:35 . This can be actually quite useful in some cases

This comment has been minimized.

@diosmosis

diosmosis Oct 1, 2020
Member

What I meant was since cdt is meant to force the datetime, it doesn't really make sense to me to apply cdo to it. Ie, if someone sends a request https://.../matomo.php?idsite=1&cdt=..., and it gets queued, then we resend it as https://.../matomo.php?idsite=1&cdt=...&cdo=..., shouldn't the cdt be respected as is? Ie, we only send the cdt when we want to force the date time to something specific, but cdo should only be applied to the timestamp if we are setting it to now.

This comment has been minimized.

@tsteur

tsteur Oct 1, 2020
Author Member

In that case someone would simply not set the cdo? I have no big preference though. It could be generally useful though when eg replaying requests maybe for example. Not sure. Right now not seeing really any downside because they don't have to set both?

This comment has been minimized.

@diosmosis

diosmosis Oct 2, 2020
Member

I guess it depends on whether the system setting cdo needs to be aware of when cdt should be respected or not.

@tsteur
Copy link
Member Author

@tsteur tsteur commented Oct 1, 2020

and also it caches piwik.js/matomo.js? is this needed or can it just be cached via an http header?

It can't really be cached via http header in case you are fully offline etc. Otherwise the approach is correct 👍

@tsteur
Copy link
Member Author

@tsteur tsteur commented Oct 1, 2020

@diosmosis applies few changes. Would otherwise love to get this merged to see if it really works and what problems there are in general. Eg I'm not an indexeddb / service worker so I'm sure there might be few things that maybe won't work as expected or could cause race conditions or so maybe.

@diosmosis diosmosis merged commit 2807e94 into 4.x-dev Oct 2, 2020
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
@diosmosis diosmosis deleted the m9939 branch Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants

level de log mis à 1

Récupération du document https://github.com/matomo-org/matomo/pull/15970.

Récupération de l'URL https://github.com/matomo-org/matomo/pull/15970...

Raccourcis

Commandes

Fermer