Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[React@18 failing test] visualize app - new charts library visualize area charts date histogram when no time filter interval errors should show error when calendar interval invalid #196303

Closed
Dosant opened this issue Oct 15, 2024 · 1 comment · Fixed by #196308
Labels
bug Fixes for quality problems that affect the customer experience Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@Dosant
Copy link
Contributor

Dosant commented Oct 15, 2024

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

Image


I investigated the problem and understand what is not working, but was hesistant to suggest a fix. I'd like a team to take another look and fix carefully.

To Reproduce:

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

React@18 failure:

error.mov

The error is thrown from here in the reducer:

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:

if (!isDurationInterval(this._i)) {
throw new TypeError('"' + this._i + '" is not a valid interval.');
}

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 there was no error anymore, so it never poped up during rendering.

The root cause of inconsitent behaviour is here:

if (buckets) return buckets;
buckets = new TimeBuckets({
'histogram:maxBars': getConfig(UI_SETTINGS.HISTOGRAM_MAX_BARS),
'histogram:barTarget': getConfig(UI_SETTINGS.HISTOGRAM_BAR_TARGET),
dateFormat: getConfig('dateFormat'),
'dateFormat:scaled': getConfig('dateFormat:scaled'),
});
updateTimeBuckets(this, calculateBounds, buckets);
return buckets;

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 and make sure an error is handled

@Dosant Dosant added bug Fixes for quality problems that affect the customer experience Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Oct 15, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations (Team:Visualizations)

@Dosant Dosant closed this as completed in feb5b79 Oct 22, 2024
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Oct 22, 2024
…ize area charts date histogram when no time filter interval errors should show error when calendar interval invalid (elastic#196308)

## Summary

close elastic#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.

(cherry picked from commit feb5b79)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
3 participants