Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
*/

import React from 'react';
import SearchBar from './search_bar';
import SearchBar, { SearchBarProps, SearchBarState, SearchBarUI } from './search_bar';
import { BehaviorSubject } from 'rxjs';
import { KibanaContextProvider } from '@kbn/kibana-react-plugin/public';
import { indexPatternEditorPluginMock as dataViewEditorPluginMock } from '@kbn/data-view-editor-plugin/public/mocks';
Expand Down Expand Up @@ -366,4 +366,48 @@ describe('SearchBar', () => {
true
);
});

describe('SearchBarUI.getDerivedStateFromProps', () => {
it('should not return the esql query if props.query doesnt change but loading state changes', () => {
const nextProps = {
query: { esql: 'test' },
isLoading: false,
} as unknown as SearchBarProps;
const prevState = {
currentProps: {
query: { esql: 'test' },
},
query: { esql: 'test_edited' },
isLoading: true,
} as unknown as SearchBarState;

const result = SearchBarUI.getDerivedStateFromProps(nextProps, prevState);
// if the query was returned, it would overwrite the state in the underlying ES|QL editor
expect(result).toEqual({
currentProps: { isLoading: false, query: { esql: 'test' } },
});
});
it('should return the query if props.query and loading state changes', () => {
const nextProps = {
query: { esql: 'test_new_props' },
isLoading: false,
} as unknown as SearchBarProps;
const prevState = {
currentProps: {
query: { esql: 'test' },
},
query: { esql: 'test_edited' },
isLoading: true,
} as unknown as SearchBarState;

const result = SearchBarUI.getDerivedStateFromProps(nextProps, prevState);
// here it makes sense to return the query, because the props.query has changed
expect(result).toEqual({
currentProps: { isLoading: false, query: { esql: 'test_new_props' } },
query: {
esql: 'test_new_props',
},
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ export interface SearchBarOwnProps<QT extends AggregateQuery | Query = Query> {
export type SearchBarProps<QT extends Query | AggregateQuery = Query> = SearchBarOwnProps<QT> &
SearchBarInjectedDeps;

interface State<QT extends Query | AggregateQuery = Query> {
export interface SearchBarState<QT extends Query | AggregateQuery = Query> {
isFiltersVisible: boolean;
openQueryBarMenu: boolean;
showSavedQueryPopover: boolean;
Expand All @@ -157,9 +157,9 @@ interface State<QT extends Query | AggregateQuery = Query> {
dateRangeTo: string;
}

class SearchBarUI<QT extends (Query | AggregateQuery) | Query = Query> extends Component<
export class SearchBarUI<QT extends (Query | AggregateQuery) | Query = Query> extends Component<
SearchBarProps<QT> & WithEuiThemeProps,
State<QT | Query>
SearchBarState<QT | Query>
> {
public static defaultProps = {
showQueryMenu: true,
Expand All @@ -177,7 +177,7 @@ class SearchBarUI<QT extends (Query | AggregateQuery) | Query = Query> extends C

public static getDerivedStateFromProps(
nextProps: SearchBarProps,
prevState: State<AggregateQuery | Query>
prevState: SearchBarState<AggregateQuery | Query>
) {
if (isEqual(prevState.currentProps, nextProps)) {
return null;
Expand All @@ -204,7 +204,13 @@ class SearchBarUI<QT extends (Query | AggregateQuery) | Query = Query> extends C
query: '',
language: nextProps.query.language,
};
} else if (nextProps.query && !isOfQueryType(nextProps.query)) {
} else if (
nextProps.query &&
isOfAggregateQueryType(nextProps.query) &&
nextProps.query.esql !== get(prevState, 'currentProps.query.esql')
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the main change, we mimic what KQL does. before this change, for ESQL the query was overwritten every time, since it was not checked if the new props value differs from the old one. Now this works.

I tried to cover this change with a unit test, but I couldn't make it work. Since on re-render the currentProps.query.esql, was not kept. This made the unit test pointless 🤷

I mean, we could refactor this part to a unit test, to a pure function. Open to apply this change if valued reviewers want to have it. Or have other ideas

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not overly worried about it tbh, but I wonder if it's possible to test the rendered output in an RTL test? I might not understand exactly what was making it hard to test though. Otherwise a separate function like you mentioned would work if you're feeling proactive.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I wanted to do this, I've tried and it didn't work, it might be me of course

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found a way to cover this in a simple way

32cc820

first test fails when I remove the fix

) {
// this code is just overriding the query with a new one in case the query has changed in props
// without the props check it would override any edits to the query, if e.g. results were returned and isLoading switches from true to false
nextQuery = nextProps.query;
}

Expand Down Expand Up @@ -248,14 +254,9 @@ class SearchBarUI<QT extends (Query | AggregateQuery) | Query = Query> extends C
/*
Keep the "draft" value in local state until the user actually submits the query. There are a couple advantages:

1. Each app doesn't have to maintain its own "draft" value if it wants to put off updating the query in app state
until the user manually submits their changes. Most apps have watches on the query value in app state so we don't
want to trigger those on every keypress. Also, some apps (e.g. dashboard) already juggle multiple query values,
each with slightly different semantics and I'd rather not add yet another variable to the mix.

2. Changes to the local component state won't trigger an Angular digest cycle. Triggering digest cycles on every
Comment thread
kertal marked this conversation as resolved.
keypress has been a major source of performance issues for us in previous implementations of the query bar.
See https://github.com/elastic/kibana/issues/14086
Each app doesn't have to maintain its own "draft" value if it wants to put off updating the query in app state
until the user manually submits their changes. Some apps have watches on the query value in app state so we don't
want to trigger those on every keypress.
*/
public state = {
isFiltersVisible: true,
Expand All @@ -265,7 +266,7 @@ class SearchBarUI<QT extends (Query | AggregateQuery) | Query = Query> extends C
query: this.props.query ? { ...this.props.query } : undefined,
dateRangeFrom: get(this.props, 'dateRangeFrom', 'now-15m'),
dateRangeTo: get(this.props, 'dateRangeTo', 'now'),
} as State<QT>;
} as SearchBarState<QT>;

public isDirty = () => {
if (!this.props.showDatePicker && this.state.query && this.props.query) {
Expand Down