Skip to content

Commit

Permalink
[React@18 failing test] fix visualize app - new charts library visual…
Browse files Browse the repository at this point in the history
…ize area charts date histogram when no time filter interval errors should show error when calendar interval invalid (#196308)

## Summary

close #196303

We're working on upgrading Kibana to React@18 (in Legacy Mode). There
are a couple failing tests when running React@18 in Legacy mode and this
is one of them

visualize app - new charts library visualize area charts date histogram
when no time filter interval errors should show error when calendar
interval invalid.
[Failure](https://buildkite.com/elastic/kibana-pull-request/builds/236562#019222ec-e8d1-4465-ada3-fd923283b6f4)


![Image](https://github.com/user-attachments/assets/2cdaca3c-9ccf-4208-b6f3-6975588eb5fe)


---- 


I investigated the problem and understand what is not working and
suggesting this simple fix, but open to any other approaches or
suggestions.


To Reproduce the failing tests: 

1. Create aggregation based viz, e.g. Area Chart
2. Add data histogram agg
3. Change minimum interval to a invalid value (for example, "f")
4. Change minimum interval to another invalid value ("ff")

React@18 failure: 


https://github.com/user-attachments/assets/f8684b48-fb24-4500-a762-2a116ed55297

The error is thrown from here in the reducer: 


https://github.com/elastic/kibana/blob/23e0e1e61c6df451cc38763b53a6e2db5518b9f4/src/plugins/vis_default_editor/public/components/sidebar/state/reducers.ts#L82

When we try to update the state using 2nd invalid value, the error is
thrown when we try to serialize the current agg with previous invalid
value.

This code is exececuted when we call `agg.serialize`:
 

https://github.com/elastic/kibana/blob/5ed698182887e18d2aa6c4b6782cc636a45a1472/src/plugins/data/common/search/aggs/buckets/lib/time_buckets/time_buckets.ts#L200-L202


**Why don't we see this failure in React@17?**

In React@17 we don't see an error screen, but we only see a log in the
console.

> TypeError: "f" is not a valid interval.

It turns out that React@17 consistently executed that reducer twice.
first time during dispatch and second time during rendering. This
shouldn't be a problem because reducers are supposed to be pure (without
side-effects). **But in this case calling `agg.serialize` only throws an
error when called the first time**! So in React@17 the reducer was
called the first time, the error was swallowed, then it was called the
2nd time and, since the `TimeBucket` was cached, there was no error
anymore, so it never bubbled up during rendering.

The root cause of inconsitent behaviour is here:


https://github.com/elastic/kibana/blob/8afbbc008222dee377aab568a639466d49c56306/src/plugins/data/common/search/aggs/buckets/date_histogram.ts#L111-L121

when `get()` called first time we create buckets and cache them. but we
cache them before calling `updateTimeBuckets` which is where the error
happens.


To fix this issue, we should make the reducer pure. One approach is to
swallow that error so that the call to `agg.serialize()` is consistent.
Another approach could be to always throw that error, but then a larger
refactor is needed and this likely a more risky and impactfull change.
  • Loading branch information
Dosant authored Oct 22, 2024
1 parent b495c37 commit feb5b79
Showing 1 changed file with 8 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,14 @@ export const getDateHistogramBucketAgg = ({
dateFormat: getConfig('dateFormat'),
'dateFormat:scaled': getConfig('dateFormat:scaled'),
});
updateTimeBuckets(this, calculateBounds, buckets);

try {
updateTimeBuckets(this, calculateBounds, buckets);
} catch (e) {
// swallow the error even though the agg is misconfigured
// eslint-disable-next-line no-console
console.error(e);
}

return buckets;
},
Expand Down

0 comments on commit feb5b79

Please sign in to comment.