JS Offline tracking #15970
JS Offline tracking #15970
Conversation
tsteur
added
the
Pull Request WIP
tsteur
added
Needs Review
There was a problem hiding this 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; | ||
| }); | ||
| }); | ||
| }) |
There was a problem hiding this comment.
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;
});
There was a problem hiding this comment.
👍 makes sense. We can tweak code later once we get feedback the overall idea actually works
| @@ -498,6 +504,10 @@ protected function getCustomTimestamp() | |||
| $cdt = strtotime($cdt); | |||
| } | |||
|
|
|||
| if (!empty($cdo)) { | |||
| $cdt = $cdt - abs($cdo); | |||
| } | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I guess it depends on whether the system setting cdo needs to be aware of when cdt should be respected or not.
It can't really be cached via http header in case you are fully offline etc. Otherwise the approach is correct 👍 |
|
@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. |
refs #9939
early stage... usage:
within a service worker do
to register service worker:
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.