-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
JS Offline tracking #15970
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.