[App Search] EnginesLogic + minor UX fix

[cee-chen]

Summary

So, I originally started this PR because I realized there was a semi-annoying race condition with with the Engines Overview page, which was this:

• Open Engines Overview page
• The meta engine API call finishes before the source engines API call, which triggers isLoading being set to false
• The page flashes an empty engines state before loading in an engines table once the source engines API call finishes

The solution to this is to only set isLoading to false for the source engines API call and not the meta engines API call, and there's quick and dirty ways to accomplish that, but I realized quickly that I should be converting this view to use Kea logic.

For some context, this was one of the first views created/migrated and predated our import/usage of Kea in Kibana, that's why it is the way it is :) So this update is long overdue and will in fact be needed for the Engines Creation views that Byron will be starting with when he hops on the Kibana train.

Checklist

• Engines Overview page still works as before
• Unit or functional tests were updated or added to match the most common scenarios

[cee-chen]

This is not a 1:1 translation from ent-search's EnginesLogic b/c the view/table component is actually fairly different from the standalone UI. Quick sparknotes of changes:

• Meta engines is on this view as well, not just source engines
• Flash messages is now a global state, not per-view
• We currently don't have engines filtering in, I'm not sure if this is something Davey eventually wants post-MVP

We're also missing a delete engine/create engine listener which I assume we'll add at some point (I guess create engine could use a different logic file actually on second thought?)

[cee-chen]

@JasonStoltz follow up to our convo here in #86822 (comment) - I actually don't mind / like the onX function naming the more I use it - let me know what you think or if you still strongly prefer something else for a naming scheme?

[JasonStoltz]

I actually think I like it also. It's just very clear, no ambiguity. +1

[cee-chen]

One super quick change to this pattern/naming convention - I remembered that Kea uses loadX for its fns and additionally also has a whole plugin for this called kea-loaders, so I decided to copy that and change fetchX and onFetchY to loadX and onLoadY.

This way if we ever use the kea-loaders, it'll be a pretty smooth naming transition!

[cee-chen]

We generally seem to use dataLoading over isLoading everywhere else in the repo so I changed this to match for consistency.

[cee-chen]

I left the actual test blocks of this component as-is, I just updated the mocks and assertions to match the new Kea usage.

[cee-chen]

@byronhulcher - this is how we've been writing our Kea logic file tests. Jason set up this architecture/pattern all the way back when the credentials logic was migrated, and it's been serving us well so far. It's extremely explicit but as a result it's easy to read/scan and write. Here's an example pattern (with exx:

  describe('actions', () => {
    // We don't have a separate block for 'listeners' but recently I've been debating that we should

    describe('someAction', () => {
      describe('aValueTheActionAffects', () => {
        it('should be set to the provided value', () => { 
          // mount the logic file, run the action, assert the value
        });
      });
      describe('anotherValueTheActionAffects', () => {
        it('should be set to some value', () => { 
          // mount the logic file, run the action, assert the value
        });
      });
    });

    describe('someOtherAction', () => {
      describe('aValueTheActionAffects', () => {
        it('should be set to the provided value', () => { 
          // mount the logic file, run the action, assert the value
        });
      });
    });

  });

  describe('selectors', () => {
    // We do have a separate section for selectors if we're testing those

    describe('someSelector', () => {
      describe('the selector changes to X when value does Y', () => {
        // ...
      });
    });

  });

LMK if that makes sense or feel free to comment/ask questions!

[JasonStoltz]

I also think it's served us fairly well. There's still some room for improvement. I think in the future, it'd be nice if we keep the action blocks as we do now, but try to consolidate the number of reducer blocks.

Each action test would start with the following reducer block:

    describe('onEnginesFetch', () => {
        it('should set state', () => {
          mount();
          EnginesLogic.actions.onEnginesFetch({ engines: [], total: 0 });

          expect(EnginesLogic.values).toEqual({
            engines: [],
            dataLoading: false,
          });
    }

If we have reducers with multiple code paths, then we can spin up additional reducer blocks:

    describe('onEnginesFetch', () => {
        it('should set state', () => {
          mount();
          EnginesLogic.actions.onEnginesFetch({ engines: [], total: 0 });

          expect(EnginesLogic.values).toEqual({
            engines: [],
            dataLoading: false,
          });
        }

        describe('engines', () => {
          it('won't be set if xyz is true ', () => {
            mount();
            EnginesLogic.actions.onEnginesFetch({ engines: [], total: 0, xyz: true });

            expect(EnginesLogic.values).toEqual({
              ...DEFAULT_VALUES,
              engines: undefined,
            });
        }
    }

Just thinking out loud here.

[cee-chen]

I like that a lot! Huge +1

[cee-chen]

Jason also implemented this method of expecting/asserting against ALL SomeLogic.values (as opposed to just SomeLogic.values.specificValues), as a way of regression testing against reducer changes that we make and forget to write tests/assertions for. It's been working really well just IMO and has helped us catch bugs.

[cee-chen]

@elasticmachine merge upstream

[byronhulcher]

These changes make sense to me, I'm going to check out locally

[byronhulcher]

I'm glad this has ended up in the kea logic now. We've discussed elsewhere that I'm not a huge fan of dataLoading/isLoading props, and prefer using null defaults for values we load from elsewhere, and doing explicit null checks for those values. I think that is a "typescript"-er way of doing it. Not a blocker for this PR, but wanted to mention my perspective on this again.

[cee-chen]

👍 I don't have super strong feelings one way or another, either an isLoading prop of some kind of a falsey check is fine/reads the same to me. What I do have stronger feelings on is preferring to remain consistent across the codebase rather than have several different / confusing instances of how we handle loading state, so if possible I'd like to tackle all instances of dataLoading in one fell swoop post-MVP.

[JasonStoltz]

I really like the explicitness of dataLoading. It could be a selector that checks for certain values to be null.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: e065361

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
enterpriseSearch	1069	1070	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
enterpriseSearch	1.8MB	1.8MB	-1.1KB

History

• 💚 Build #98040 succeeded 2d25a5f1840472a1b5cfd7c8927ea5c08cda1b92
• 💚 Build #97415 succeeded b0d0d41d4f6c99da6881d000580a55dc03a5df2a
• 💔 Build #97410 failed 424fde8f36bea28e7e099a7c1e998484bad8e0ad

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

[cee-chen]

@byronhulcher If this PR mostly looks good to you, do you mind give it an approval? Thanks!