[EuiSearchBar] Make onChange args type simple, as before

[acstll]

Summary

This is reverting a change introduced in #9142 for the EuiSearchBarOnChangeArgs type, where the query prop could be null also when there was no error, requiring additional checks in consumer code.

Why are we making this change?

To prevent type errors in consumer code.

Before (in #9142)

interface ArgsWithQuery {
  query: Query;
  queryText: string;
  error: null;
}

interface ArgsWithPlainText {
  query: null;
  queryText: string;
  error: null;
}

interface ArgsWithError {
  query: null;
  queryText: string;
  error: Error;
}

export type EuiSearchBarOnChangeArgs = ArgsWithQuery | ArgsWithPlainText | ArgsWithError;

with the above, query could be null also when error is null (no error); so checking for !args.error is not enough to ensure query not being null

After

interface ArgsWithQuery {
  query: Query;
  queryText: string;
  error: null;
}

interface ArgsWithError {
  query: null;
  queryText: string;
  error: Error;
}

export type EuiSearchBarOnChangeArgs = ArgsWithQuery | ArgsWithError;

now when error is null, query has a value different than null 👍 (as it used to be)

-export type EuiSearchBarOnChangeArgs = ArgsWithQuery | ArgsWithPlainText | ArgsWithError;
+export type EuiSearchBarOnChangeArgs = ArgsWithQuery | ArgsWithError;

Impact to users

🟡 Changes required. Any usages of EuiSearchBarProps for the search prop in EuiInMemoryTable need to be replaced with the new EuiInMemoryTableSearchBarProps, as the onChange type is different between them.

Thank you @mgadewoll for preparing these:

• Update needed in Kibana
• Update needed in Cloud-UI 🔒

QA

General checklist

• Browser QA
  • Checked in both light and dark modes
  • Checked in both MacOS and Windows high contrast modes
    • (emulate forced colors if you do not have access to a Windows machine.)
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Checked for accessibility including keyboard-only and screenreader modes
• Docs site QA
  • Added documentation
  • Props have proper autodocs (using @default if default values are missing) and playground toggles
  • Checked Code Sandbox works for any docs examples
• Code quality checklist
  • Added or updated jest and cypress tests
  • Updated visual regression tests
• Release checklist
  • A changelog entry exists and is marked appropriately
  • If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • If the changes unblock an issue in a different repo, smoke tested carefully (see Testing EUI features in Kibana ahead of time)
• Designer checklist
  • If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)

[mgadewoll]

We need to update this. Since we're changing types here consumers that so far rely on the EuiSearchBarProps instead of the new EuiInMemoryTableSearchBarProps will need to update as the onChange type is different between them.

These are the required internal changes:

• Kibana
• Cloud-UI

[acstll]

@mgadewoll updated the changelog so it's a minor instead of a patch in 08d6dc6 — suggestions welcome!

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: 08d6dc6
• Documentation website
• Storybook

History

• 💚 Build #2130 succeeded 7a775f2
• 💚 Build #2129 succeeded 2fcb133
• 💚 Build #2126 succeeded efa7c1c
• 💚 Build #2125 succeeded c29d8a7

cc @acstll

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: 08d6dc6

History

• 💚 Build #5361 succeeded 7a775f2
• 💚 Build #5360 succeeded 2fcb133
• 💚 Build #5357 succeeded efa7c1c

cc @acstll

[mgadewoll]

Imho, we should an entry for the updated search prop type that uses EuiInMemoryTableSearchBarProps now.
And considering that this will require updates to implementations due to an API change (though small), I'd even mark it as breaking.

[acstll]

now when error is null, query has a value different than null 👍 (as it used to be)

👆 this (that I wrote in the description above) is actually not true, because whenever EuiInMemoryTable is using searchFormat="text" it will pass { query: null, queryText: 'something', error: null } down to EuiSearchBar, regardless.

after gathering feedback in a couple of discussions, I'm leaning towards thinking that the original change in #9142 that added ArgsWithPlainText is actually OK, and that the new checks required e.g. (!args.error && args.query) as in https://github.com/elastic/kibana/pull/244866/files are a better path forward than the ones needed for this PR, which also means another breaking change.

if you agree, I'd close this and put up another PR with just a few JSDoc additions… — @mgadewoll what do you think?

[mgadewoll]

if you agree, I'd close this and put up another PR with just a few JSDoc additions… — @mgadewoll what do you think?

Agreed, that sounds sensible. The current approach works fine. And as you say, if the EuiSearchBar does receive query: null now, the earlier type is ok 👍

[acstll]

for reasons stated above, let's close this in favor of only improving JSDoc around the current types — thank you @mgadewoll @weronikaolejniczak @tkajtoch 🙏