From 490c630c9afbf43a5b02769336248d324d1e6546 Mon Sep 17 00:00:00 2001
From: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Date: Tue, 22 Oct 2024 22:39:47 +1100
Subject: [PATCH] [8.x] [React@18 failing test] fix visualize app - new charts
 library visualize area charts date histogram when no time filter interval
 errors should show error when calendar interval invalid (#196308) (#197198)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[React@18 failing test] fix visualize app - new charts library
visualize area charts date histogram when no time filter interval errors
should show error when calendar interval invalid
(#196308)](https://github.com/elastic/kibana/pull/196308)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Anton
Dosov","email":"anton.dosov@elastic.co"},"sourceCommit":{"committedDate":"2024-10-22T09:50:15Z","message":"[React@18
failing test] fix visualize app - new charts library visualize area
charts date histogram when no time filter interval errors should show
error when calendar interval invalid (#196308)\n\n##
Summary\r\n\r\nclose
https://github.com/elastic/kibana/issues/196303\r\n\r\nWe're working on
upgrading Kibana to React@18 (in Legacy Mode). There\r\nare a couple
failing tests when running React@18 in Legacy mode and this\r\nis one of
them\r\n\r\nvisualize app - new charts library visualize area charts
date histogram\r\nwhen no time filter interval errors should show error
when calendar\r\ninterval
invalid.\r\n[Failure](https://buildkite.com/elastic/kibana-pull-request/builds/236562#019222ec-e8d1-4465-ada3-fd923283b6f4)\r\n\r\n\r\n![Image](https://github.com/user-attachments/assets/2cdaca3c-9ccf-4208-b6f3-6975588eb5fe)\r\n\r\n\r\n----
\r\n\r\n\r\nI investigated the problem and understand what is not
working and\r\nsuggesting this simple fix, but open to any other
approaches or\r\nsuggestions.\r\n\r\n\r\nTo Reproduce the failing tests:
\r\n\r\n1. Create aggregation based viz, e.g. Area Chart\r\n2. Add data
histogram agg\r\n3. Change minimum interval to a invalid value (for
example, \"f\")\r\n4. Change minimum interval to another invalid value
(\"ff\")\r\n\r\nReact@18 failure:
\r\n\r\n\r\nhttps://github.com/user-attachments/assets/f8684b48-fb24-4500-a762-2a116ed55297\r\n\r\nThe
error is thrown from here in the reducer:
\r\n\r\n\r\nhttps://github.com/elastic/kibana/blob/23e0e1e61c6df451cc38763b53a6e2db5518b9f4/src/plugins/vis_default_editor/public/components/sidebar/state/reducers.ts#L82\r\n\r\nWhen
we try to update the state using 2nd invalid value, the error
is\r\nthrown when we try to serialize the current agg with previous
invalid\r\nvalue.\r\n\r\nThis code is exececuted when we call
`agg.serialize`:\r\n
\r\n\r\nhttps://github.com/elastic/kibana/blob/5ed698182887e18d2aa6c4b6782cc636a45a1472/src/plugins/data/common/search/aggs/buckets/lib/time_buckets/time_buckets.ts#L200-L202\r\n\r\n\r\n**Why
don't we see this failure in React@17?**\r\n\r\nIn React@17 we don't see
an error screen, but we only see a log in the\r\nconsole.\r\n\r\n>
TypeError: \"f\" is not a valid interval.\r\n\r\nIt turns out that
React@17 consistently executed that reducer twice.\r\nfirst time during
dispatch and second time during rendering. This\r\nshouldn't be a
problem because reducers are supposed to be pure
(without\r\nside-effects). **But in this case calling `agg.serialize`
only throws an\r\nerror when called the first time**! So in React@17 the
reducer was\r\ncalled the first time, the error was swallowed, then it
was called the\r\n2nd time and, since the `TimeBucket` was cached, there
was no error\r\nanymore, so it never bubbled up during
rendering.\r\n\r\nThe root cause of inconsitent behaviour is
here:\r\n\r\n\r\nhttps://github.com/elastic/kibana/blob/8afbbc008222dee377aab568a639466d49c56306/src/plugins/data/common/search/aggs/buckets/date_histogram.ts#L111-L121\r\n\r\nwhen
`get()` called first time we create buckets and cache them. but
we\r\ncache them before calling `updateTimeBuckets` which is where the
error\r\nhappens.\r\n\r\n\r\nTo fix this issue, we should make the
reducer pure. One approach is to\r\nswallow that error so that the call
to `agg.serialize()` is consistent.\r\nAnother approach could be to
always throw that error, but then a larger\r\nrefactor is needed and
this likely a more risky and impactfull
change.","sha":"feb5b79a96162b0259ecfd5d8216050cd074a3a4","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:SharedUX","backport:prev-minor"],"title":"[React@18
failing test] fix visualize app - new charts library visualize area
charts date histogram when no time filter interval errors should show
error when calendar interval
invalid","number":196308,"url":"https://github.com/elastic/kibana/pull/196308","mergeCommit":{"message":"[React@18
failing test] fix visualize app - new charts library visualize area
charts date histogram when no time filter interval errors should show
error when calendar interval invalid (#196308)\n\n##
Summary\r\n\r\nclose
https://github.com/elastic/kibana/issues/196303\r\n\r\nWe're working on
upgrading Kibana to React@18 (in Legacy Mode). There\r\nare a couple
failing tests when running React@18 in Legacy mode and this\r\nis one of
them\r\n\r\nvisualize app - new charts library visualize area charts
date histogram\r\nwhen no time filter interval errors should show error
when calendar\r\ninterval
invalid.\r\n[Failure](https://buildkite.com/elastic/kibana-pull-request/builds/236562#019222ec-e8d1-4465-ada3-fd923283b6f4)\r\n\r\n\r\n![Image](https://github.com/user-attachments/assets/2cdaca3c-9ccf-4208-b6f3-6975588eb5fe)\r\n\r\n\r\n----
\r\n\r\n\r\nI investigated the problem and understand what is not
working and\r\nsuggesting this simple fix, but open to any other
approaches or\r\nsuggestions.\r\n\r\n\r\nTo Reproduce the failing tests:
\r\n\r\n1. Create aggregation based viz, e.g. Area Chart\r\n2. Add data
histogram agg\r\n3. Change minimum interval to a invalid value (for
example, \"f\")\r\n4. Change minimum interval to another invalid value
(\"ff\")\r\n\r\nReact@18 failure:
\r\n\r\n\r\nhttps://github.com/user-attachments/assets/f8684b48-fb24-4500-a762-2a116ed55297\r\n\r\nThe
error is thrown from here in the reducer:
\r\n\r\n\r\nhttps://github.com/elastic/kibana/blob/23e0e1e61c6df451cc38763b53a6e2db5518b9f4/src/plugins/vis_default_editor/public/components/sidebar/state/reducers.ts#L82\r\n\r\nWhen
we try to update the state using 2nd invalid value, the error
is\r\nthrown when we try to serialize the current agg with previous
invalid\r\nvalue.\r\n\r\nThis code is exececuted when we call
`agg.serialize`:\r\n
\r\n\r\nhttps://github.com/elastic/kibana/blob/5ed698182887e18d2aa6c4b6782cc636a45a1472/src/plugins/data/common/search/aggs/buckets/lib/time_buckets/time_buckets.ts#L200-L202\r\n\r\n\r\n**Why
don't we see this failure in React@17?**\r\n\r\nIn React@17 we don't see
an error screen, but we only see a log in the\r\nconsole.\r\n\r\n>
TypeError: \"f\" is not a valid interval.\r\n\r\nIt turns out that
React@17 consistently executed that reducer twice.\r\nfirst time during
dispatch and second time during rendering. This\r\nshouldn't be a
problem because reducers are supposed to be pure
(without\r\nside-effects). **But in this case calling `agg.serialize`
only throws an\r\nerror when called the first time**! So in React@17 the
reducer was\r\ncalled the first time, the error was swallowed, then it
was called the\r\n2nd time and, since the `TimeBucket` was cached, there
was no error\r\nanymore, so it never bubbled up during
rendering.\r\n\r\nThe root cause of inconsitent behaviour is
here:\r\n\r\n\r\nhttps://github.com/elastic/kibana/blob/8afbbc008222dee377aab568a639466d49c56306/src/plugins/data/common/search/aggs/buckets/date_histogram.ts#L111-L121\r\n\r\nwhen
`get()` called first time we create buckets and cache them. but
we\r\ncache them before calling `updateTimeBuckets` which is where the
error\r\nhappens.\r\n\r\n\r\nTo fix this issue, we should make the
reducer pure. One approach is to\r\nswallow that error so that the call
to `agg.serialize()` is consistent.\r\nAnother approach could be to
always throw that error, but then a larger\r\nrefactor is needed and
this likely a more risky and impactfull
change.","sha":"feb5b79a96162b0259ecfd5d8216050cd074a3a4"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/196308","number":196308,"mergeCommit":{"message":"[React@18
failing test] fix visualize app - new charts library visualize area
charts date histogram when no time filter interval errors should show
error when calendar interval invalid (#196308)\n\n##
Summary\r\n\r\nclose
https://github.com/elastic/kibana/issues/196303\r\n\r\nWe're working on
upgrading Kibana to React@18 (in Legacy Mode). There\r\nare a couple
failing tests when running React@18 in Legacy mode and this\r\nis one of
them\r\n\r\nvisualize app - new charts library visualize area charts
date histogram\r\nwhen no time filter interval errors should show error
when calendar\r\ninterval
invalid.\r\n[Failure](https://buildkite.com/elastic/kibana-pull-request/builds/236562#019222ec-e8d1-4465-ada3-fd923283b6f4)\r\n\r\n\r\n![Image](https://github.com/user-attachments/assets/2cdaca3c-9ccf-4208-b6f3-6975588eb5fe)\r\n\r\n\r\n----
\r\n\r\n\r\nI investigated the problem and understand what is not
working and\r\nsuggesting this simple fix, but open to any other
approaches or\r\nsuggestions.\r\n\r\n\r\nTo Reproduce the failing tests:
\r\n\r\n1. Create aggregation based viz, e.g. Area Chart\r\n2. Add data
histogram agg\r\n3. Change minimum interval to a invalid value (for
example, \"f\")\r\n4. Change minimum interval to another invalid value
(\"ff\")\r\n\r\nReact@18 failure:
\r\n\r\n\r\nhttps://github.com/user-attachments/assets/f8684b48-fb24-4500-a762-2a116ed55297\r\n\r\nThe
error is thrown from here in the reducer:
\r\n\r\n\r\nhttps://github.com/elastic/kibana/blob/23e0e1e61c6df451cc38763b53a6e2db5518b9f4/src/plugins/vis_default_editor/public/components/sidebar/state/reducers.ts#L82\r\n\r\nWhen
we try to update the state using 2nd invalid value, the error
is\r\nthrown when we try to serialize the current agg with previous
invalid\r\nvalue.\r\n\r\nThis code is exececuted when we call
`agg.serialize`:\r\n
\r\n\r\nhttps://github.com/elastic/kibana/blob/5ed698182887e18d2aa6c4b6782cc636a45a1472/src/plugins/data/common/search/aggs/buckets/lib/time_buckets/time_buckets.ts#L200-L202\r\n\r\n\r\n**Why
don't we see this failure in React@17?**\r\n\r\nIn React@17 we don't see
an error screen, but we only see a log in the\r\nconsole.\r\n\r\n>
TypeError: \"f\" is not a valid interval.\r\n\r\nIt turns out that
React@17 consistently executed that reducer twice.\r\nfirst time during
dispatch and second time during rendering. This\r\nshouldn't be a
problem because reducers are supposed to be pure
(without\r\nside-effects). **But in this case calling `agg.serialize`
only throws an\r\nerror when called the first time**! So in React@17 the
reducer was\r\ncalled the first time, the error was swallowed, then it
was called the\r\n2nd time and, since the `TimeBucket` was cached, there
was no error\r\nanymore, so it never bubbled up during
rendering.\r\n\r\nThe root cause of inconsitent behaviour is
here:\r\n\r\n\r\nhttps://github.com/elastic/kibana/blob/8afbbc008222dee377aab568a639466d49c56306/src/plugins/data/common/search/aggs/buckets/date_histogram.ts#L111-L121\r\n\r\nwhen
`get()` called first time we create buckets and cache them. but
we\r\ncache them before calling `updateTimeBuckets` which is where the
error\r\nhappens.\r\n\r\n\r\nTo fix this issue, we should make the
reducer pure. One approach is to\r\nswallow that error so that the call
to `agg.serialize()` is consistent.\r\nAnother approach could be to
always throw that error, but then a larger\r\nrefactor is needed and
this likely a more risky and impactfull
change.","sha":"feb5b79a96162b0259ecfd5d8216050cd074a3a4"}}]}]
BACKPORT-->

Co-authored-by: Anton Dosov <anton.dosov@elastic.co>
---
 .../data/common/search/aggs/buckets/date_histogram.ts    | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/plugins/data/common/search/aggs/buckets/date_histogram.ts b/src/plugins/data/common/search/aggs/buckets/date_histogram.ts
index 66820ff0d3e94..314ee4d03042c 100644
--- a/src/plugins/data/common/search/aggs/buckets/date_histogram.ts
+++ b/src/plugins/data/common/search/aggs/buckets/date_histogram.ts
@@ -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;
           },