Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Enterprise Search] Upgrade Kea to 2.3, update LogicMounter helper w/ props support #94232

Merged
merged 3 commits into from
Mar 10, 2021
Merged

[Enterprise Search] Upgrade Kea to 2.3, update LogicMounter helper w/ props support #94232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
82ae123
Enhance our LogicMounter helper to accept and build props
cee-chen Mar 9, 2021
8a566ec
Update logic tests in AS that require props to now use LogicMounter
cee-chen Mar 9, 2021
a71f142
Upgrade Kea to 2.3.0 for future BindLogic use
cee-chen Mar 9, 2021
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
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -712,7 +712,7 @@
"json5": "^1.0.1",
"jsondiffpatch": "0.4.1",
"jsts": "^1.6.2",
"kea": "^2.2.0",
"kea": "^2.3.0",
"keymirror": "0.1.1",
"leaflet": "1.5.1",
"leaflet-draw": "0.4.14",
Expand Down
7 changes: 5 additions & 2 deletions x-pack/plugins/enterprise_search/public/applications/__mocks__/kea.mock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,15 @@ export const setMockActions = (actions: object) => {
* const { mount, unmount } = new LogicMounter(SomeLogic);
*
* it('some test', () => {
* mount({ someValue: 'hello' });
* mount({ someValue: 'hello' }, { someProp: 'world' });
* unmount();
* });
*/
import { resetContext, Logic, LogicInput } from 'kea';

interface LogicFile {
inputs: Array<LogicInput<Logic>>;
build(props?: object): void;
mount(): Function;
}
export class LogicMounter {
Expand All @@ -110,8 +111,10 @@ export class LogicMounter {
};

// Automatically reset context & mount the logic file
public mount = (values?: object) => {
public mount = (values?: object, props?: object) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just want to quickly note - I'm not a super huge fan of how logic files that don't want defaults and only want props (e.g., AppLogic) now have to either pass undefined or {} as a first param. If I could go back in time I'd probably change this to

  public mount = ({ defaults, props }) => {

and have test files pass in their API like mount({ defaults: { foo: 'bar' } }) or mount({ props: { bar: 'baz' } }). IDK if we feel strongly to make that change in a ton of files at this point so I'm fine leaving it as-is, mostly just thinking out loud

Copy link
Contributor Author

@cee-chen cee-chen Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Also for more context, here's the first conversation where I mentioned LogicMounter needing to support props: #92560 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

I supposed you could do a mountWithProps, which would not require the empty object, but I am not a huge fan of that.

Copy link
Contributor Author

@cee-chen cee-chen Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Ya for sure, to clarify I don't hate this either and I'm fine with shipping it for now - we can definitely come back to this API later if we start using a ton more logic files w/ props!

this.resetContext(values);
if (props) this.logicFile.build(props);
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got that from Kea's docs here for context: https://kea.js.org/docs/guide/advanced#mounting-and-unmounting

// build the logic with props (`logic(props)` is short for `logic.build(props)`)


const unmount = this.logicFile.mount();
this.unmountFn = unmount;
return unmount; // Keep Kea behavior of returning an unmount fn from mount
Expand Down
15 changes: 5 additions & 10 deletions x-pack/plugins/enterprise_search/public/applications/app_search/app_logic.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,19 @@
*/

import { DEFAULT_INITIAL_APP_DATA } from '../../../common/__mocks__';

import { resetContext } from 'kea';
import { LogicMounter } from '../__mocks__';

import { AppLogic } from './app_logic';

describe('AppLogic', () => {
const mount = (props = {}) => {
AppLogic({ ...DEFAULT_INITIAL_APP_DATA, ...props });
AppLogic.mount();
};
const { mount } = new LogicMounter(AppLogic);

beforeEach(() => {
jest.clearAllMocks();
resetContext({});
});

it('sets values from props', () => {
mount();
mount({}, DEFAULT_INITIAL_APP_DATA);

expect(AppLogic.values).toEqual({
ilmEnabled: true,
Expand Down Expand Up @@ -53,7 +48,7 @@ describe('AppLogic', () => {
describe('actions', () => {
describe('setOnboardingComplete()', () => {
it('sets true', () => {
mount({ appSearch: { onboardingComplete: false } });
mount({}, { ...DEFAULT_INITIAL_APP_DATA, appSearch: { onboardingComplete: false } });

AppLogic.actions.setOnboardingComplete();
expect(AppLogic.values.account.onboardingComplete).toEqual(true);
Expand All @@ -64,7 +59,7 @@ describe('AppLogic', () => {
describe('selectors', () => {
describe('myRole', () => {
it('falls back to an empty object if role is missing', () => {
mount({ appSearch: {} });
mount({}, { ...DEFAULT_INITIAL_APP_DATA, appSearch: {} });

expect(AppLogic.values.myRole).toEqual({});
});
Expand Down
22 changes: 9 additions & 13 deletions ...pp_search/components/curations/components/curation_queries/curation_queries_logic.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@
* 2.0.
*/

import { resetContext } from 'kea';
import { LogicMounter } from '../../../../../__mocks__';

import { CurationQueriesLogic } from './curation_queries_logic';

describe('CurationQueriesLogic', () => {
const { mount } = new LogicMounter(CurationQueriesLogic);

const MOCK_QUERIES = ['a', 'b', 'c'];

const DEFAULT_PROPS = { queries: MOCK_QUERIES };
Expand All @@ -19,18 +21,12 @@ describe('CurationQueriesLogic', () => {
hasOnlyOneQuery: false,
};

const mount = (props = {}) => {
CurationQueriesLogic({ ...DEFAULT_PROPS, ...props });
CurationQueriesLogic.mount();
};

beforeEach(() => {
jest.clearAllMocks();
resetContext({});
});

it('has expected default values passed from props', () => {
mount();
mount({}, DEFAULT_PROPS);
expect(CurationQueriesLogic.values).toEqual(DEFAULT_VALUES);
});

Expand All @@ -42,7 +38,7 @@ describe('CurationQueriesLogic', () => {

describe('addQuery', () => {
it('appends an empty string to the queries array', () => {
mount();
mount(DEFAULT_VALUES);
CurationQueriesLogic.actions.addQuery();

expect(CurationQueriesLogic.values).toEqual({
Expand All @@ -55,7 +51,7 @@ describe('CurationQueriesLogic', () => {

describe('deleteQuery', () => {
it('deletes the query string at the specified array index', () => {
mount();
mount(DEFAULT_VALUES);
CurationQueriesLogic.actions.deleteQuery(1);

expect(CurationQueriesLogic.values).toEqual({
Expand All @@ -67,7 +63,7 @@ describe('CurationQueriesLogic', () => {

describe('editQuery', () => {
it('edits the query string at the specified array index', () => {
mount();
mount(DEFAULT_VALUES);
CurationQueriesLogic.actions.editQuery(2, 'z');

expect(CurationQueriesLogic.values).toEqual({
Expand All @@ -81,15 +77,15 @@ describe('CurationQueriesLogic', () => {
describe('selectors', () => {
describe('hasEmptyQueries', () => {
it('returns true if queries has any empty strings', () => {
mount({ queries: ['', '', ''] });
mount({}, { queries: ['', '', ''] });

expect(CurationQueriesLogic.values.hasEmptyQueries).toEqual(true);
});
});

describe('hasOnlyOneQuery', () => {
it('returns true if queries only has one item', () => {
mount({ queries: ['test'] });
mount({}, { queries: ['test'] });

expect(CurationQueriesLogic.values.hasOnlyOneQuery).toEqual(true);
});
Expand Down
8 changes: 4 additions & 4 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18212,10 +18212,10 @@ kdbush@^3.0.0:
resolved "https://registry.yarnpkg.com/kdbush/-/kdbush-3.0.0.tgz#f8484794d47004cc2d85ed3a79353dbe0abc2bf0"
integrity sha512-hRkd6/XW4HTsA9vjVpY9tuXJYLSlelnkTmVFu4M9/7MIYQtFcHpbugAU7UbOfjOiVSVYl2fqgBuJ32JUmRo5Ew==

kea@^2.2.0:
version "2.2.0"
resolved "https://registry.yarnpkg.com/kea/-/kea-2.2.0.tgz#1ba4a174a53880cca8002a67cf62b19b30d09702"
integrity sha512-IzgTC6SC89wTLfiBMPlduG4r4YanxONYK4werz8RMZxPvcYw4XEEK8xQJguwVYtLCEGm4x5YiLCubGqGfRcbEw==
kea@^2.3.0:
version "2.3.3"
resolved "https://registry.yarnpkg.com/kea/-/kea-2.3.3.tgz#8fbd6d0c4ba5079c5abe46486bbc7dc1fd071a62"
integrity sha512-NZQHisfEvlg+e6BsHckW03IYaIBY+fuK4xiov7ShZ0GudUmNLhqgHSxUsykU/wdrCPEI6ANX1gyDIRTnUd3HyA==

kew@~0.1.7:
version "0.1.7"
Expand Down