search

algolia / search-insights-gtm

Google Tag Manager template for detecting front-end Algolia search metrics
Apache License 2.0
7 stars 5 forks source link

fix: never skip init #39

Closed JasonBerry closed 6 months ago

JasonBerry commented 6 months ago

Due to this block if the apiKey and appId aren't set an error is thrown, so we can check for that to see if init has been called on the page already or by the tag.

If this call is successful it will result in a 422 from the Insights API, but I would like to change search-insights.js to not send an empty list of events.

JasonBerry commented 6 months ago

If this call is successful it will result in a 422 from the Insights API, but I would like to change search-insights.js to not send an empty list of events.

Can you say this differently, I don't quite understand what you mean.

The API will return a 422 because the list of events being sent is an empty array. I'm making a change to search-insights.js to not make requests in this case, because there's no point in hitting the API with these empty requests, we can just fail fast instead.

What was the original problem this is fixing? Sorry if it's obvious.

At the moment if search-insights.js is already found on the page then we use that copy instead of loading another one and don't call its init method because we're assuming that it's already been called in the page. It's become clear over a number of support issues that this is not the case though, and so exceptions are thrown by the sendEvents method because no apiKey and appId have been specified. This fix tries to call that same method first to see if an apiKey and appId have been set, and if not then call init to set them.

I feel a bit nervous about using a real API call to check if things are configured. Won't that artificially inflate various metrics across the place? Also thinking about things like on-call if all of a sudden every insights-gtm website visitor sends a dummy sendEvent call.

I'll bump the version of search-insights.js containing my "fast fail" in this release too, so that this is unlikely to happen. I've also changed it to only send a dummy event if we've detected that search-insights.js has already been loaded into the page (this won't be the case for most insights-gtm customers).

JasonBerry commented 6 months ago

Relevant PR for search-insights.js here. https://github.com/algolia/search-insights.js/pull/532

JasonBerry commented 6 months ago

I had to change the approach, as in end-to-end testing on live GTM I found that try/catch isn't supported, so we'll just always init now. This shouldn't be a problem as the GTM setInWindow method won't override the object on the window if it already exists.

JasonBerry commented 6 months ago

In the original version of this you called sendEvents wrapped in a try/catch in order to trigger the exception path in insights.js here: https://github.com/algolia/search-insights.js/pull/532/files#diff-ec66e9c2e8feaf0f060c2abc34d08b51c6c4e05640d10f7d287fac40915569c6L20, right?

If so, and because GTM doesn't allow try catch, is that suggesting that we just need a new method on aa that tells you the info you're after? Then you can a) not overload/use sendEvents and b) you can just use normal logic in GTM rather than a try/catch.

Not sure if I've misunderstood.

I went too high-tech on this I've realised in testing. We can simply always init and this shouldn't cause a problem. copyFromWindow deep copies the 'aa' object it finds on window if it exists (which loses the apiKey and appId that were set in that context). If it doesn't exist we load in search-insights.js, get the 'aa' object and in all cases run init to ensure that the apiKey and appId are set. setInWindow tries to set aa on the window if it doesn't exist so that it can be used externally.