fix: fix the check for presence of Event

[zubko]

Why

The check for undefined by using a function will fail on React Native, as Event will be needed to be accessed to pass it as an argument, and it's not defined.

Such check will fail in the browser as well:

What

Use the "classic" check instead

Checklist

• Tests added
• Changelog updated
• Documentation updated

[CLAassistant]

All committers have signed the CLA.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[codecapitano]

Hey @zubko,

thanks a lot for your contribution, that's awesome!

Question:
The ReferenceError above is because there's no variable with name.
If we pass a variable with value of undefined or undefined directly it works as expected.

Can you help me a bit to understand the React Native specific problem?

[zubko]

Hi @codecapitano

React Native doesn't have an Event constructor defined in the global scope:

https://snack.expo.dev/@zubko/checking-event-api-on-rn

In React Native:

There is also a notion of events. But more like the events on React level - we have "Synthetic Events", but there is no DOM API which has the "real events".

Yes, you are right, if it was at least smth like global.Event = undefined then the check would work. This can be a workaround to use Faro Core, if we put this line to our code. But I think the correct check is to assume that it may be completely missing. Like the check for the window when we SSR, we do just typeof window === 'undefined', we can't pass window to a function, or it will crash Node.

In Web:

On Node.js global scope has Event defined, so the existing check in the Core works fine.

I was able to use Faro Core on RN to send logs to our Grafana proxy (I was even able to use the FetchTransport from the Faro Web without making changes to it)

But I needed to patch this line in the Core to make it work.

I didn't dive too deep yet. How it will be different to not have this Event constructor defined, I don't know. But at least the logs were sent and received, so it's a good start, and I feel optimistic :)

[codecapitano]

Hey @zubko

Thanks heaps for the detailed explanation, that great!

Yes, you are right, if it was at least smth like global.Event = undefined then the check would work. This can be a workaround to use Faro Core, if we put this line to our code.

Oh of course Faro should capture this problem so you do not have to do this kind of workaround.

So i think this is a good solution 🙌

[codecapitano]

@zubko Would you mind telling us about your journey in using Faro with RN?
Would be very interesting for the team.

[codecapitano]

Thanks a lot for the improvement @zubko 🤝

[zubko]

Hi @codecapitano

Thanks for approving the PR!

Would you mind telling us about your journey in using Faro with RN?

So far it's super basic. It was just a "spike", if we can use Faro to send logs to Grafana from Mobile app. Using Faro Web directly was throwing some errors of missing APIs that didn't give me much confidence to continue with it. So I just used Faro Core and then initialized it with FetchTransport from Faro Web, and then I sent a couple of logs and found them on Grafana. For the stack trace parser I just return an empty array etc. So the most basic integration.

The real implementation will be next, for example for the stack trace parser it's good that we just have one Hermes JS Engine on RN for both platforms, so I will do it in regards to that engine. In the worst case it also won't be a problem to not have a stack trace, because we still plan to use the native crashlog solution, as some of the crashes on RN are native and it's not possible to catch them from JS in any way.

The general idea so far is to send logs, errors and events to Grafana for observability and to share the dashboards with the backend and to start to collect custom metrics, like app loading time etc. We will send crashes and errors to the separate mobile crashlog solution as well for deeper analysis. And eventually maybe we will get to the point to have some OTEL support and to pass some traceId from the client, but this is a very long term plan at this point.

So far we have more plans and wishes than code. But thanks a lot for being so responsive on the PR. It gives us also more confidence to continue with this direction. If we get to the point of having something more tangible I will let you know.

[codecapitano]

HI @zubko

Oh wow that's cool thanks a lot!

My teammate @eskirk started some experiments with RN instrumentation in case you want to have a look.

So far we have more plans and wishes than code.

This is very valuable information, so please drop them here :)
Generally speaking, RN instrumentation is a very often requested feature.
Currently it's not on our roadmap due to a ton of conflicting priorities.

We are super open for feedback, ideas and of course contributions.

But thanks a lot for being so responsive on the PR

My/our pleasure. It's always amazing to get get contributions from the community ❤️

cc @cedricziel

[eskirk]

hi @zubko - this is really awesome, thanks for the contribution.

my react native instrumentation was a bit hacky, but definitely got the job done for our hackathon. as for this issue in particular, I was looking into some kind of polyfill for react native Event type - for some reason I thought there were more issues than just this undefined check... regardless - this is a welcome improvement.

let us know if you ever have any questions - I am hoping that we can start to dedicate some real time to mobile instrumentations in the near future 🤞🏼 with enough interest and engagement from the community this will definitely become something that gets prioritized.