Fix metric undefined #2589
Fix metric undefined #2589
Conversation
BundleMonFiles updated (1)
Unchanged files (7)
Total files change -22B 0% Final result: ✅ View report in BundleMon website ➡️ |
|
I can't seem to reproduce this locally on EDIT: my bad, I wasn't on the realtime graph |
| @@ -437,6 +435,7 @@ export default class VisitorGraph extends React.Component { | |||
| api.get(`/api/stats/${encodeURIComponent(this.props.site.domain)}/top-stats`, this.props.query) | |||
| .then((res) => { | |||
| this.setState({ topStatsLoadingState: LOADING_STATE.loaded, topStatData: res }) | |||
| this.resetMetric() | |||
There was a problem hiding this comment.
Why move this from componentDidUpdate to the API response callback?
There was a problem hiding this comment.
I did this because the top stat labels can only change when we fetch new data (the labels are returned from the API). And the only condition where we want to "reset the metric" is when the labels have changed.
This will avoid having to write this (or something similar) in componentDidUpdate
const topStatLabels = topStatData && topStatData.top_stats.map(({ name }) => METRIC_MAPPING[name]).filter(name => name)
const prevTopStatLabels = prevState.topStatData && prevState.topStatData.top_stats.map(({ name }) => METRIC_MAPPING[name]).filter(name => name)
if (topStatLabels && `${topStatLabels}` !== `${prevTopStatLabels}`) {
//.....
}(There's another function called updateMetric which is used when a new metric is selected)
There was a problem hiding this comment.
Makes sense. In this case I think it would be best to run it as a callback to setState like so:
this.setState({ topStatsLoadingState: LOADING_STATE.loaded, topStatData: res }, this.resetMetrics)There was a problem hiding this comment.
Done in f1ef1f4
Changes
This PR fixes a bug introduced in #2560. For a reproduction of the bug, go to this dashboard URL and remove the
Signup.logged_in is falsefilter. After doing so:ApiError: ** (CaseClauseError) no case clause matching: :undefinedThis is because
metricbecomes undefined in theVisitorGraphstate when setting it totopStatLabels[0]on this line (iftopStatLabelsis an empty array).This PR makes sure that we're not relying on the top stat labels when setting the metric. It also does some refactoring to make the code more readable. Reviewing commit-by-commit suggested.
Tests
Changelog
Documentation
Dark mode