fix(SFINT-5395): Wrapped sendEvent response in try/catch in analyticsFetchClient

[SimonMilord]

SFINT-5395

Wrapped the sendEvent fetch reponse in a try/catch block to prevent some issues we were encountering when the response would fail on the fetch call.

For example, we had an issue where an ad blocker (Ublock) would prevent the analytics event when changing tab in the Salesforce HIP and would display an error modal.

[mmitiche]

were you able to test this?

[erocheleau]

I would put the try catch only around the fetch specifically.

Suggested change

	try {
	const response = await fetch(url, fetchData);
	try {
	const response = await fetch(url, fetchData);
	} catch (error) {
	console.error('An error has occured when sending the event.', error);
	}

Because they way you have it now, you'll catch the other thrown error on line 42 and you'll potentially display a weird message.
You'd catch this:

throw new Error(
                    `An error has occurred when sending the "${eventType}" event. Check the console logs for more details.`
                );
                ```

[SimonMilord]

If I do it like that then the rest of the code in the sendEvent method does not have access to response

[rphilippen]

I do want to point out that @erocheleau is putting it lightly with "I would". You should not have put the try-catch around the whole code, including the existing error handling. Referring to my previous comment, you effectively disabled most error handling (Promise rejects with error) and clobbered it into "Promise resolves with undefined", which is a completely new state integrators will have to deal with. This can't be the intent.

As a style remark, the fact that this is now relying on an implicit return undefined since there is no final return statement is bad form in my opinion. To me that's an indication that we're not dealing with the error, but just shoving it downstream via the "success" (resolve) path.

[erocheleau]

1 change which I think is better error handling.

[rphilippen]

You need to explain in your PR description why we should introduce this API breaking change, or we should reject this in my opinion.

Update: after checking I realized the top level signature is already Promise<AnyEventResponse | void> for the beacon client, so it's not breaking (although I do think you should type it as void not undefined). The remark about not clobbering current error handling still stands though. You can achieve that by putting the try-catch just around the fetch, as already suggested, and adding an explicit return. To be clear, this is the pattern to be able to access the response outside of the try-catch still:

        let response;
        try {
            response = await fetch(url, fetchData);
        } catch (error) {
            console.error('An error has occured when sending the event.', error);
            return;
        }

I have to say, the "this may return something, it may not" API is something I'm not fond of. Also, I still think you should justify in the PR description why it's a good thing to always suppress errors from fetch. Customers may have misconfigured something. With this change it'll just be logging a generic error to the console, instead of rejecting the promise with the root cause.

[rphilippen]

TL;DR: This is an API breaking change: it means all (integrator) code invoking this method now suddenly has to deal with undefined as well.

Your PR description does explain what you are trying to solve (the error warning), but not clearly how you want to solve this. Putting a try-catch around something is the technical thing you do, but error handling is more than that. It is deciding how you respond to the error; and I feel this is not really explained in the PR description.

Your aim seems to be to suppress an error on fetch. However, returning undefined from a method that used to return a response object (containing visitId and visitorId properties) is not suppressing the error. You've now added a third state:

1. Promise resolves with response
2. Promise rejects with error
3. Promise resolves with undefined (caused indirectly by an error).

Suppressing fetch errors in a non API-breaking way, means mapping it to either option 1 or 2. I don't think you should fake a visitId and visitorId, so that basically leaves you with rejecting… which already happened before this PR, because an async function automatically catches errors. The only thing you could do is change the message to something more generic than whatever fetch gives.

[rphilippen]

I do want to point out that @erocheleau is putting it lightly with "I would". You should not have put the try-catch around the whole code, including the existing error handling. Referring to my previous comment, you effectively disabled most error handling (Promise rejects with error) and clobbered it into "Promise resolves with undefined", which is a completely new state integrators will have to deal with. This can't be the intent.

As a style remark, the fact that this is now relying on an implicit return undefined since there is no final return statement is bad form in my opinion. To me that's an indication that we're not dealing with the error, but just shoving it downstream via the "success" (resolve) path.

[SimonMilord]

You need to explain in your PR description why we should introduce this API breaking change, or we should reject this in my opinion.

Update: after checking I realized the top level signature is already Promise<AnyEventResponse | void> for the beacon client, so it's not breaking (although I do think you should type it as void not undefined). The remark about not clobbering current error handling still stands though. You can achieve that by putting the try-catch just around the fetch, as already suggested, and adding an explicit return. To be clear, this is the pattern to be able to access the response outside of the try-catch still:

        let response;
        try {
            response = await fetch(url, fetchData);
        } catch (error) {
            console.error('An error has occured when sending the event.', error);
            return;
        }

I have to say, the "this may return something, it may not" API is something I'm not fond of. Also, I still think you should justify in the PR description why it's a good thing to always suppress errors from fetch. Customers may have misconfigured something. With this change it'll just be logging a generic error to the console, instead of rejecting the promise with the root cause.

Hey Ruben!
These are very valid points, thank you for bringing it up. I do like your suggestion and I changed the type to void and wrap the fetch in the try/catch block.

As for the error message/PR description, the issue was brought up by a Coveo Service product expert who was experiencing issues with analytics throwing when his adblocker was enabled, which would filter out analytics events. In my opinion, a lot of users could have adblockers enabled so it is something worth addressing.

What would you prefer? We could make the error message maybe less generic or if you know of a way to silence the error only when adblockers are enabled somehow im open to suggestions.

[erocheleau]

@rphilippen I think it becomes a question of where we draw the line of scope for this lib then.

Do we want the lib to "hide the fact that fetch threw" and expose an easy to digest console.error? (off the top of my head I don't know many other cases other than network errors/adblockers that would make the fetch throw)

Or do we want to push that error handling to the user of the library to have to wrap this sendEvent with a try catch themselves?

Personally since we're already doing some error handling here, I feel like this is the right place to handle this potential fetch that throws.

Are you okay with the new changes proposed?

[erocheleau]

Seems better this way to me.

[rphilippen]

@erocheleau put it correctly that it comes down to defining the scope of the (error handling of the) library. That was basically was what my "complaint" was about; the PR description didn't really justify this change to hide failures from fetch in my opinion. With the additional comments, explaining the rationale and behavior compared to other libraries, plus the code changes, I'm fully on board with this solution.