Skip to content
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

tracker updates #1272

Merged
merged 1 commit into from Aug 4, 2022
Merged

tracker updates #1272

merged 1 commit into from Aug 4, 2022

Conversation

Maxime-J
Copy link
Contributor

@Maxime-J Maxime-J commented Jul 7, 2022

As fetch was used to replace navigator.sendBeacon, it might be good to handle all the requests with it.
It's more convenient than XHR.

If you're okay, this PR :

  • replaces XHR by Fetch
  • centralizes the requests (no call to collect api without passing through collect())
  • adds a parameter to the collect function, for requests which needs to persist on exit/location change,
    like it is currently the case with link events (and it can probably be useful for possible future uses)
  • applies the same logic in sendEvent() as in other functions

if (!persist) {
fetch(endpoint, options).then(res => res.text()).then(resText => { cache = resText });
} else {
options['keepalive'] = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, all the requests should use the keepalive option. Previously, the other requests were sent using XHR, therefore those requests were not persisted.

Copy link
Contributor

@rohandebsarkar rohandebsarkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be the structure for fetch requests.

Whether to cache the response could also be checked by type === 'pageview', but the previous one is probably better if any other responses need to cached in the future.

@@ -73,17 +58,22 @@ import { removeTrailingSlash } from '../lib/url';
return a;
};

const collect = (type, payload) => {
const collect = (type, payload, persist = false) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const collect = (type, payload, persist = false) => {
const collect = (type, payload, cacheResponse = false) => {

if (cache) headers['x-umami-cache'] = cache;
let options = {
method: 'POST',
body: JSON.stringify({type, payload}),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
body: JSON.stringify({type, payload}),
body: JSON.stringify({type, payload}),
keepalive: true,

body: JSON.stringify({type, payload}),
headers
};
if (!persist) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!persist) {
if (cacheResponse) {

if (!persist) {
fetch(endpoint, options).then(res => res.text()).then(resText => { cache = resText });
} else {
options['keepalive'] = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
options['keepalive'] = true;

assign(getPayload(), {
event_type,
event_value,
}),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}),
})

event_type,
event_value,
}),
true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
true

@@ -93,7 +83,7 @@ import { removeTrailingSlash } from '../lib/url';
website: uuid,
url,
referrer,
}),
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
})
}),
true

@Maxime-J
Copy link
Contributor Author

The suggested changes differs a lot from the initial PR, which keeps exactly the same behaviour as now:

Standard requests not resistant to location change/exit, like in XHR, and the answer always cached locally
Specific requests (currently for link events) resistant to location change/exit, like in navigator.sendBeacon, without handling the answer cache.

@rohandebsarkar
Copy link
Contributor

Yeah, okay. I'll probably work this on another PR if @mikecao is interested.

@mikecao
Copy link
Collaborator

mikecao commented Jul 12, 2022

I'm still undecided on this because the reason we went with XHR is to support older browsers. I haven't read up on the state of fetch support yet. Maybe we can create a separate tracker to support older browsers?

@rohandebsarkar
Copy link
Contributor

I understand, but fetch() is currently supported by all the major browsers, except Internet Explorer. But Internet Explorer has already been declared retired on 15 June 2022.

Reference: https://developer.mozilla.org/en-US/docs/Web/API/fetch#browser_compatibility

@Maxime-J
Copy link
Contributor Author

Understood, actually went with fetch because it was introduced in tracker in a previous commit.
And as Rohan said, it's widely supported now.

I also believe that kind of error wouldn't happen (#1246), not sure though.

@mikecao mikecao merged commit c9e966d into umami-software:master Aug 4, 2022
@Maxime-J Maxime-J deleted the tracker-fetch branch August 11, 2022 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants