Fix null analytics reporter #1841
Conversation
src/core/core.js
Outdated
| @@ -94,7 +95,7 @@ export default class Core { | |||
| * A local reference to the analytics reporter, used to report events for this component | |||
| * @type {AnalyticsReporter} | |||
| */ | |||
| this._analyticsReporter = config.analyticsReporter; | |||
| this._analyticsReporter = config.analyticsReporter || new AnalyticsReporter(); | |||
There was a problem hiding this comment.
What do you think about falling back to the NoopAnalyticsReporter instead? If we were to create the AnalyticsReporter like this, it wouldn't be configured properly because no params are passed to the constructor
There was a problem hiding this comment.
i thought NoopAnalyticsReporter does nothing when you call report(event); it simply returns true. so, if we fall back on it, and then try to report an event as we are doing on line 184, it will just report true, which is not the behavior i believe we want.
There was a problem hiding this comment.
My thought process is that if an analyticsReporter isn't present on the config, then we should use the NoopAnalyticsReporter in order to not send any analytics events. That way we won't run into an error about the analytics reporter being null. If we do want the analytics reporter to actually send the event, we should look into why the config.analyticsReporter isn't set
There was a problem hiding this comment.
that makes sense! i will use NoopAnalyticsReporter then. thanks!
There was a problem hiding this comment.
do we have any idea why analytics reporter was missing when we do local development?
There was a problem hiding this comment.
i traced it to answers-search-ui/src/answers-umd.js looks like it _analyticsReporterService was initialized as null. and then on lines 257 to 260, it didn't end up with a value probably because it didn't meet the conditions in those lines. i think it makes sense to not send any analytics in core.js if somehow, the value being passed through is null. the interface allows for that.
There was a problem hiding this comment.
yeah so I think we want to understand why it was initialized as null. It appears to work in the platform, so there is something different when you use the code editor vs when you do local development. It seems like if we can determine that then it might work. @cea2aj do you think it might be related to the api domain logic? I suspect that most developers would expect to be able to test analytics locally, which is why I would prefer to not just use the noop reporter.
There was a problem hiding this comment.
I think it's more likely something other than the api domain logic. Have we double checked that the businessId is set in the config? That's the only thing that affects the _eligibleForAnalytics variable in answers-umd.js. I think it's still worth using the NoopAnalyticsReporter in answers-umd if _eligibleForAnalytics is false
There was a problem hiding this comment.
looks like this. _eligibleForAnalytics is the culprit. core config needs businessId, but it looks like nationwide's global_config.json for that site didn't have businessId set. once i set it and did another run, the original issue disappeared. i will abandon this cr. thanks everyone.
No description provided.