Fix potential access to deprecated web API's

[Hamish-taylor]

The problem

This is the timing object, and on this line we are accessing .responseEnd which has been deprecated in favor of this. We do actually collect this timing object when we get the performance metrics here, but I don't fully understand what happens with that information.

The result

The result of this is that the events are still sent, but due to missing/bad data the events are never displayed on the frontend.

The fix

• Added checks to make sure we are not accessing the undefined property. <- there's probably a better way of handling this
• Allowed the new timing object to be used

My current questions

• How do we ensure the fix is backwards compatible
  • According to caniuse.com The 'new' metric is supported across 93.12% or users compared to 95.71% with the old method (a difference of 2.59%).
• Do the new timing events measure the same things?
  • From my testing it appears that they do

[Olwiba]

Nice work on this & nice write-up. Appreciate the links to make review easier 😄
I've added some comments/thoughts

[Olwiba]

can you tell me more about this change?

[Hamish-taylor]

Well firstly I just noticed that its spelt wrong 😅 This is mostly unrelated to the primary issue that the PR is trying to solve. However, the captureMissingRequests function uses the offset var which I had modified, so when testing I wanted to verify that I hadn't broken it. This is when I found that in this function _rum seems to always be undefined which results in captureMissingRequests never being set. Adding it into the delayedExecutionFunctions means that it is run after the _rum object is created. Now that I think about it this might result in the first page load missing any of these requests as the property might be set after the occur? 🤔

[Olwiba]

hmm, possibly something to test - it makes sense to delay this as it's calling the _rum.captureMissingRequests and if _rum is always null then it wouldn't have been doing anything anyway. I do see a null check inside the captureMissingRequests, I wonder if this was to mitigate this

[Olwiba]

Thanks for the changes - The code changes are looking good to me.
Please make sure you test this thoroughly & pre-soak because I've not had a chance to test this out myself 🚀

If you're unsure or would like some help with testing feel free to reach out otherwise I'll leave it in your hands

[darcythomas]

Looks like you have a broken test.

Other than that it looks good :P

[darcythomas]

LGTM

[PanosNB]

I did a code review with Hamish who explained the work to me. Everything made sense to me: I suggested he adds in some comments to explain the ideas.

Overall, I understood the following:

• In raygun.loader.js, we register a new callback to the new navigationPerformanceObserver object, if it exists, that will call our onLoadHandler(), indicating page loaded, after the navigation event has been triggered. The latter is achieved adding to the observer the name of the desired entry type
• In index.js, because the new way of measuring starts the timing from 0 rather the current UNIX timestamp that the old object uses, we set the timestamp offset to 0 for both virtual pages and the new way but to the current timestamp for the old way
• Also in index.js, instead of directly accessing fields that might not exist, a bridge pattern is used that stores either the or the old type of object in the timing reference. Then, a method getTimingDuration() requests the right type of fields per type
• Finally, also in index.js, we exclude navigation and visibility-state objects from being further investigated as it is enough to be reported at the top level of the session: adding them to secondary, resulted into nans

[Olwiba]

Optional: A possible clarity improvement here could be to split this check out into a variable so it's obvious what the containing code is for.

Something like (var name could be improved):

var supportsPerformanceNavigationEntries =  !!window.PerformanceObserver && !!window.PerformanceObserver.supportedEntryTypes && window.PerformanceObserver.supportedEntryTypes.includes('navigation');

Then the conditional is simplified to:

else if (supportsPerformanceNavigationEntries)...

[Olwiba]

I've reviewed the code changes since the last review & nothing stands out to me as off.
Great work here 🚀

Leaving approval for others, but I'm happy with the update

[xenolightning]

Great work @Hamish-taylor!

The only comment I had was to maybe bump minor version, but not required

[xenolightning]

Update versions