[SIEM] Migrating frontend services to NP

[rylnd]

Summary

This is a first pass at migrating some services over to their new platform equivalents.
Partially addresses #45831.

Removed legacy imports:

• ui/index_patterns
• ui/documentation_links
• ui/chrome
  • settings client
  • getBasePath
  • helpExtension
  • breadcrumbs
  • getXsrfToken
• ui/registry/feature_catalogue

Other changes:

• replaced useKibanaUiSetting hook with kibana_react's useUiSetting and useUiSetting$
• replaced useKibanaCore and useKibanaPlugins with kibana_react's useKibana
• added mocks for kibana_react

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

• This was checked for cross-browser compatibility, including a check against IE11
  - [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
  - [ ] Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios
• This was checked for keyboard-only and screenreader accessibility

For maintainers

• This was checked for breaking API changes and was labeled appropriately
• This includes a feature addition or change that requires a release note and was labeled appropriately

[FrankHassanabad]

I think we are the only ones left with react-apollo at this point :-)

I see some left overs in Infra but I would imagine we are going to take over ownership unless another plugin starts using it.

[FrankHassanabad]

👍 Nice to see the new API. Like it takes a <string[]> as a template.

[rylnd]

What do you think about exporting "bound" generic types internally?

const indexPatterns = useUiSetting<string[]>(DEFAULT_INDEX_KEY);
// vs
const indexPatterns = useIndexPatterns();

In both cases indexPatterns would be typed as string[]. I already did that for our own useKibana hook to reduce boilerplate, and I think it makes sense for these common settings that we retrieve all over the place, too (DEFAULT_INDEX_KEY, DEFAULT_KBN_VERSION).

[FrankHassanabad]

I think the bound version const indexPatterns = useIndexPatterns(); reads much better

[rylnd]

NB this new typed API has so far surfaced one type error that I was excited might be a bug, but the client/endpoint that ultimately use it seem to handle the array interpolation just fine? 🤷‍♂

Also, we don't seem to have consistent jargon for this concept of an array of index patterns: it's mainly called defaultIndex(not plural?), but I've also seen defaultIndexPattern, indicesConfig, etc. Is the lack of pluralization due to the fuzziness of the elastic API? Is an array of index patterns also an index pattern?

[FrankHassanabad]

I think it's influenced by the ES API which in most cases takes a variable called index which is an array.

[rylnd]

FYI: It looks like this broad context might be replaced by more declarative hooks in the near future, so wrapping our current usage is definitely the way to go given our simple use case.

[FrankHassanabad]

Are there two different ways of doing this?

I see both:

defaultIndex: kibana.services.uiSettings.get<string[]>(DEFAULT_INDEX_KEY),

and

useUiSetting<string[]>(DEFAULT_INDEX_KEY),

[FrankHassanabad]

Looks like maybe one is a React Hook function and the other is just a plain API call for non-tsx React code?

[rylnd]

They both reference the same context provider under the hood! The former's an HOC prop (via withKibana) used for class components, and the latter is a hook used by function components.

[rylnd]

@FrankHassanabad you bring up a good point, though: I'm trying to pull as much of this as possible from the react context and avoiding grabbing state (e.g. chrome.getUiSettingsClient) outside of react/platform's control.

[FrankHassanabad]

That's an interesting type & WithKibanaProps; That's nice we have more access to Kibana Props now 👍

[FrankHassanabad]

Checked it out, tested it, looked through it. Soo many good things in here to reject it over anything small in my opinion so I give this an early exclusive LGTM from myself. I understand there is probably a few more questions from others in their expertise.

I did test out some of the detection engine rules and everything looks ok there as well.

[rylnd]

@rudolf we have a few uses of chrome.getUiSettingsClient remaining in functions that we use to populate part of our initial redux state (time filter preferences, etc.). Here's how we're thinking of tackling that, just wanted a sanity check from your end:

1. Define the setup() method of our plugin
2. Fetch those UI settings in setup, and do some merging with the rest of our static initial state to create our store object
3. Pass that store to the store Provider (via a private plugin property?) during the start() phase

[joshdover]

Step 1 & 2 sound fine, however step 3 is a bit off. Registering applications is done during setup via the core.application.register API. You'll be able to use your initial store right there without using a private property on your plugin class.

[joshdover]

Because you're already using Redux, I heavily advise not using the KibanaContextProvider pattern and instead managing all state through Redux abstractions. Not to say this way isn't viable, it's just going to be rough from a maintenance perspective.

See this discussion for more context: #53029 (comment)

[joshdover]

I believe you can replace all this code with the NP mounting mechanism in setup: core.application.register

[joshdover]

Step 1 & 2 sound fine, however step 3 is a bit off. Registering applications is done during setup via the core.application.register API. You'll be able to use your initial store right there without using a private property on your plugin class.

[joshdover]

What error are you getting? Why do you set these settings here? Did you mean to use core.uiSettings.get?

[rylnd]

Because you're already using Redux, I heavily advise not using the KibanaContextProvider pattern and instead managing all state through Redux abstractions. Not to say this way isn't viable, it's just going to be rough from a maintenance perspective.

See this discussion for more context: #53029 (comment)

@joshdover yep, I've been watching that conversation and realize that the context provider isn't a good long-term solution (as evidenced by all the simple components that we're still mocking).

Do you have a more complete example of using redux to accomplish all this? Things I'm wondering about:

• How would a component access e.g. the saved queries service?
• How would a component subscribe to UI Settings changes?

A call would be great to make sure that we're on the same page as platform, too.

That being said, are you okay with merging this in the meantime? I've already started on a PR that fleshes out a setup step and removes a lot of the cruft you mentioned, but this PR mainly just removes our custom providers/hooks/mocks.

[joshdover]

Do you have a more complete example of using redux to accomplish all this? Things I'm wondering about:

No examples that I'm aware of but here's the flow I imagined...

• How would a component access e.g. the saved queries service?

Components wouldn't access saved queries directly, they'd dispatch a GET_SAVED_QUERY actionCreator which would use redux-thunk or redux-saga to do the actual fetching, then dispatch a SET_SAVED_QUERY action with the details needed. A reducer would then handle updating the Redux store, propagating changes to React components.

• How would a component subscribe to UI Settings changes?

Similarly, a subscription could be set up with the uiSettings APIs directly, and when the settings change, dispatch an appropriate action to the Redux store. A reducer would be set up to handle these actions and compute the next state of the store.

import { combineLatest } from 'rxjs';

combineLatest(
  uiSettings.get$('setting1'),
  uiSettings.get$('setting2')
).subscribe(([setting1, setting2]) => {
  store.dispatch({
    type: 'UPDATE_SETTINGS',
    setting1,
    setting2
  });
});

[joshdover]

That being said, are you okay with merging this in the meantime? I've already started on a PR that fleshes out a setup step and removes a lot of the cruft you mentioned, but this PR mainly just removes our custom providers/hooks/mocks.

Yep, no opposition here, it's your plugin. Just trying to point out ways to make refactoring easier in the future.

[rylnd]

Thanks for the help @joshdover, it's much appreciated.

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 512a949

History

• 💚 Build #16489 succeeded 26a6d0e
• 💚 Build #16286 succeeded 83f4eb4
• 💚 Build #16242 succeeded c517c89
• 💚 Build #16192 succeeded c5e9fbb
• 💔 Build #16177 failed ad83050

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[spalger]

This is somehow breaking master, reverting for now