Skip to content

Commit

Permalink
Fix Typescript errors (#16)
Browse files Browse the repository at this point in the history
* Fix public mocks

* Fix empty states types

* Fix engine table component errors

* Fix engine overview component errors

* Fix setup guide component errors

- SetBreadcrumbs will be fixed in a separate commit

* Fix App Search index errors

* Fix engine overview header component errors

* Fix applications context index errors

* Fix kibana breadcrumb helper errors

* Fix license helper errors

* ❗ Refactor React Router EUI link/button helpers
- in order to fix typescript errors

- this changes the component logic significantly to a react render prop, so that the Link and Button components can have different types - however, end behavior should still remain the same

* Fix telemetry helper errors

* Minor unused var cleanup in plugin files

* Fix telemetry collector/savedobjects errors

* Fix MockRouter type errors and add IRouteDependencies export

- routes will use IRouteDependencies in the next few commits

* Fix engines route errors

* Fix telemetry route errors

* Remove any type from source code

- thanks to Scotty for the inspiration

* Add eslint rules for Enterprise Search plugin

- Add checks for type any, but only on non-test files
- Disable react-hooks/exhaustive-deps, since we're already disabling it in a few files and other plugins also have it turned off

* Cover uncovered lines in engines_table and telemetry tests
  • Loading branch information
Constance authored and cee-chen committed Jun 26, 2020
1 parent a2815ca commit b059247
Show file tree
Hide file tree
Showing 44 changed files with 300 additions and 185 deletions.
12 changes: 12 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -905,6 +905,18 @@ module.exports = {
},
},

/**
* Enterprise Search overrides
*/
{
files: ['x-pack/plugins/enterprise_search/**/*.{ts,tsx}'],
excludedFiles: ['x-pack/plugins/enterprise_search/**/*.{test,mock}.{ts,tsx}'],
rules: {
'react-hooks/exhaustive-deps': 'off',
'@typescript-eslint/no-explicit-any': 'error',
},
},

/**
* disable jsx-a11y for kbn-ui-framework
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { mockLicenseContext } from './license_context.mock';
*
* const wrapper = mountWithContext(<Component />, { enterpriseSearchUrl: 'someOverride', license: {} });
*/
export const mountWithContext = (children, context) => {
export const mountWithContext = (children: React.ReactNode, context?: object) => {
return mount(
<I18nProvider>
<KibanaContext.Provider value={{ ...mockKibanaContext, ...context }}>
Expand All @@ -40,7 +40,7 @@ export const mountWithContext = (children, context) => {
*
* Same usage/override functionality as mountWithContext
*/
export const mountWithKibanaContext = (children, context) => {
export const mountWithKibanaContext = (children: React.ReactNode, context?: object) => {
return mount(
<KibanaContext.Provider value={{ ...mockKibanaContext, ...context }}>
{children}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { mockKibanaContext } from './kibana_context.mock';
import { mockLicenseContext } from './license_context.mock';

jest.mock('react', () => ({
...jest.requireActual('react'),
...(jest.requireActual('react') as object),
useContext: jest.fn(() => ({ ...mockKibanaContext, ...mockLicenseContext })),
}));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,11 @@ const { intl } = intlProvider.getChildContext();
*
* const wrapper = shallowWithIntl(<Component />);
*/
export const shallowWithIntl = (children) => {
return shallow(<I18nProvider>{children}</I18nProvider>, {
context: { intl },
childContextTypes: { intl },
})
export const shallowWithIntl = (children: React.ReactNode) => {
const context = { context: { intl } };

return shallow(<I18nProvider>{children}</I18nProvider>, context)
.childAt(0)
.dive()
.dive(context)
.shallow();
};
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { EngineOverviewHeader } from '../engine_overview_header';

import './empty_states.scss';

export const EmptyState: React.FC<> = () => {
export const EmptyState: React.FC = () => {
const { enterpriseSearchUrl, http } = useContext(KibanaContext) as IKibanaContext;

const buttonProps = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ describe('NoUserState', () => {
});

it('renders with username', () => {
getUserName.mockImplementationOnce(() => 'dolores-abernathy');
(getUserName as jest.Mock).mockImplementationOnce(() => 'dolores-abernathy');

const wrapper = shallowWithIntl(<NoUserState />);
const prompt = wrapper.find(EuiEmptyPrompt).dive();
const description1 = prompt.find(FormattedMessage).at(1).dive();
Expand All @@ -62,7 +63,7 @@ describe('EmptyState', () => {

button.simulate('click');
expect(sendTelemetry).toHaveBeenCalled();
sendTelemetry.mockClear();
(sendTelemetry as jest.Mock).mockClear();
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { EngineOverviewHeader } from '../engine_overview_header';

import './empty_states.scss';

export const ErrorState: ReactFC<> = () => {
export const ErrorState: React.FC = () => {
const { enterpriseSearchUrl } = useContext(KibanaContext) as IKibanaContext;

return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { EngineOverviewHeader } from '../engine_overview_header';

import './empty_states.scss';

export const LoadingState: React.FC<> = () => {
export const LoadingState: React.FC = () => {
return (
<EuiPage restrictWidth className="engine-overview empty-state">
<SetBreadcrumbs isRoot />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { getUserName } from '../../utils/get_username';

import './empty_states.scss';

export const NoUserState: React.FC<> = () => {
export const NoUserState: React.FC = () => {
const username = getUserName();

return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,15 @@ import '../../../__mocks__/react_router_history.mock';

import React from 'react';
import { act } from 'react-dom/test-utils';
import { render } from 'enzyme';
import { render, ReactWrapper } from 'enzyme';

import { I18nProvider } from '@kbn/i18n/react';
import { KibanaContext } from '../../../';
import { LicenseContext } from '../../../shared/licensing';
import { mountWithContext, mockKibanaContext } from '../../../__mocks__';

import { EmptyState, ErrorState, NoUserState } from '../empty_states';
import { EngineTable } from './engine_table';
import { EngineTable, IEngineTablePagination } from './engine_table';

import { EngineOverview } from './';

Expand All @@ -25,7 +25,7 @@ describe('EngineOverview', () => {
it('isLoading', () => {
// We use render() instead of mount() here to not trigger lifecycle methods (i.e., useEffect)
// TODO: Consider pulling this out to a renderWithContext mock/helper
const wrapper = render(
const wrapper: Cheerio = render(
<I18nProvider>
<KibanaContext.Provider value={{ http: {} }}>
<LicenseContext.Provider value={{ license: {} }}>
Expand Down Expand Up @@ -85,7 +85,7 @@ describe('EngineOverview', () => {
},
};
const mockApi = jest.fn(() => mockedApiResponse);
let wrapper;
let wrapper: ReactWrapper;

beforeAll(async () => {
wrapper = await mountWithApiMock({ get: mockApi });
Expand All @@ -105,7 +105,8 @@ describe('EngineOverview', () => {
});

describe('pagination', () => {
const getTablePagination = () => wrapper.find(EngineTable).first().prop('pagination');
const getTablePagination: () => IEngineTablePagination = () =>
wrapper.find(EngineTable).first().prop('pagination');

it('passes down page data from the API', () => {
const pagination = getTablePagination();
Expand Down Expand Up @@ -156,8 +157,8 @@ describe('EngineOverview', () => {
* Test helpers
*/

const mountWithApiMock = async ({ get, license }) => {
let wrapper;
const mountWithApiMock = async ({ get, license }: { get(): any; license?: object }) => {
let wrapper: ReactWrapper | undefined;
const httpMock = { ...mockKibanaContext.http, get };

// We get a lot of act() warning/errors in the terminal without this.
Expand All @@ -166,8 +167,12 @@ describe('EngineOverview', () => {
await act(async () => {
wrapper = mountWithContext(<EngineOverview />, { http: httpMock, license });
});
wrapper.update(); // This seems to be required for the DOM to actually update
if (wrapper) {
wrapper.update(); // This seems to be required for the DOM to actually update

return wrapper;
return wrapper;
} else {
throw new Error('Could not mount wrapper');
}
};
});
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import { EngineTable } from './engine_table';

import './engine_overview.scss';

export const EngineOverview: ReactFC<> = () => {
export const EngineOverview: React.FC = () => {
const { http } = useContext(KibanaContext) as IKibanaContext;
const { license } = useContext(LicenseContext) as ILicenseContext;

Expand All @@ -45,12 +45,12 @@ export const EngineOverview: ReactFC<> = () => {
const [metaEnginesPage, setMetaEnginesPage] = useState(1);
const [metaEnginesTotal, setMetaEnginesTotal] = useState(0);

const getEnginesData = async ({ type, pageIndex }) => {
const getEnginesData = async ({ type, pageIndex }: IGetEnginesParams) => {
return await http.get('/api/app_search/engines', {
query: { type, pageIndex },
});
};
const setEnginesData = async (params, callbacks) => {
const setEnginesData = async (params: IGetEnginesParams, callbacks: ISetEnginesCallbacks) => {
try {
const response = await getEnginesData(params);

Expand All @@ -72,7 +72,7 @@ export const EngineOverview: ReactFC<> = () => {
const callbacks = { setResults: setEngines, setResultsTotal: setEnginesTotal };

setEnginesData(params, callbacks);
}, [enginesPage]); // eslint-disable-line react-hooks/exhaustive-deps
}, [enginesPage]);

useEffect(() => {
if (hasPlatinumLicense(license)) {
Expand All @@ -81,7 +81,7 @@ export const EngineOverview: ReactFC<> = () => {

setEnginesData(params, callbacks);
}
}, [license, metaEnginesPage]); // eslint-disable-line react-hooks/exhaustive-deps
}, [license, metaEnginesPage]);

if (hasErrorConnecting) return <ErrorState />;
if (hasNoAccount) return <NoUserState />;
Expand Down Expand Up @@ -150,3 +150,16 @@ export const EngineOverview: ReactFC<> = () => {
</EuiPage>
);
};

/**
* Type definitions
*/

interface IGetEnginesParams {
type: string;
pageIndex: number;
}
interface ISetEnginesCallbacks {
setResults: React.Dispatch<React.SetStateAction<never[]>>;
setResultsTotal: React.Dispatch<React.SetStateAction<number>>;
}
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,9 @@ describe('EngineTable', () => {

it('handles empty data', () => {
const emptyWrapper = mountWithContext(
<EngineTable data={[]} pagination={{ totalEngines: 0 }} />
<EngineTable data={[]} pagination={{ totalEngines: 0, pageIndex: 0, onPaginate: () => {} }} />
);
const emptyTable = wrapper.find(EuiBasicTable);
const emptyTable = emptyWrapper.find(EuiBasicTable);
expect(emptyTable.prop('pagination').pageIndex).toEqual(0);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/

import React, { useContext } from 'react';
import { EuiBasicTable, EuiLink } from '@elastic/eui';
import { EuiBasicTable, EuiBasicTableColumn, EuiLink } from '@elastic/eui';
import { FormattedMessage, FormattedDate, FormattedNumber } from '@kbn/i18n/react';
import { i18n } from '@kbn/i18n';

Expand All @@ -14,31 +14,33 @@ import { KibanaContext, IKibanaContext } from '../../../index';

import { ENGINES_PAGE_SIZE } from '../../../../../common/constants';

interface IEngineTableProps {
data: Array<{
name: string;
created_at: string;
document_count: number;
field_count: number;
}>;
pagination: {
totalEngines: number;
pageIndex: number;
onPaginate(pageIndex: number);
};
export interface IEngineTableData {
name: string;
created_at: string;
document_count: number;
field_count: number;
}
export interface IEngineTablePagination {
totalEngines: number;
pageIndex: number;
onPaginate(pageIndex: number): void;
}
export interface IEngineTableProps {
data: IEngineTableData[];
pagination: IEngineTablePagination;
}
interface IOnChange {
export interface IOnChange {
page: {
index: number;
};
}

export const EngineTable: ReactFC<IEngineTableProps> = ({
export const EngineTable: React.FC<IEngineTableProps> = ({
data,
pagination: { totalEngines, pageIndex = 0, onPaginate },
pagination: { totalEngines, pageIndex, onPaginate },
}) => {
const { enterpriseSearchUrl, http } = useContext(KibanaContext) as IKibanaContext;
const engineLinkProps = (name) => ({
const engineLinkProps = (name: string) => ({
href: `${enterpriseSearchUrl}/as/engines/${name}`,
target: '_blank',
onClick: () =>
Expand All @@ -50,13 +52,13 @@ export const EngineTable: ReactFC<IEngineTableProps> = ({
}),
});

const columns = [
const columns: Array<EuiBasicTableColumn<IEngineTableData>> = [
{
field: 'name',
name: i18n.translate('xpack.enterpriseSearch.appSearch.enginesOverview.table.column.name', {
defaultMessage: 'Name',
}),
render: (name) => (
render: (name: string) => (
<EuiLink data-test-subj="engineNameLink" {...engineLinkProps(name)}>
{name}
</EuiLink>
Expand All @@ -65,6 +67,8 @@ export const EngineTable: ReactFC<IEngineTableProps> = ({
truncateText: true,
mobileOptions: {
header: true,
// Note: the below props are valid props per https://elastic.github.io/eui/#/tabular-content/tables (Responsive tables), but EUI's types have a bug reporting it as an error
// @ts-ignore
enlarge: true,
fullWidth: true,
truncateText: false,
Expand All @@ -79,7 +83,7 @@ export const EngineTable: ReactFC<IEngineTableProps> = ({
}
),
dataType: 'string',
render: (dateString) => (
render: (dateString: string) => (
// e.g., January 1, 1970
<FormattedDate value={new Date(dateString)} year="numeric" month="long" day="numeric" />
),
Expand All @@ -93,7 +97,7 @@ export const EngineTable: ReactFC<IEngineTableProps> = ({
}
),
dataType: 'number',
render: (number) => <FormattedNumber value={number} />,
render: (number: number) => <FormattedNumber value={number} />,
truncateText: true,
},
{
Expand All @@ -105,7 +109,7 @@ export const EngineTable: ReactFC<IEngineTableProps> = ({
}
),
dataType: 'number',
render: (number) => <FormattedNumber value={number} />,
render: (number: number) => <FormattedNumber value={number} />,
truncateText: true,
},
{
Expand All @@ -117,7 +121,7 @@ export const EngineTable: ReactFC<IEngineTableProps> = ({
}
),
dataType: 'string',
render: (name) => (
render: (name: string) => (
<EuiLink {...engineLinkProps(name)}>
<FormattedMessage
id="xpack.enterpriseSearch.appSearch.enginesOverview.table.action.manage"
Expand All @@ -140,7 +144,7 @@ export const EngineTable: ReactFC<IEngineTableProps> = ({
totalItemCount: totalEngines,
hidePerPageOptions: true,
}}
onChange={({ page }): IOnChange => {
onChange={({ page }: IOnChange) => {
const { index } = page;
onPaginate(index + 1); // Note on paging - App Search's API pages start at 1, EuiBasicTables' pages start at 0
}}
Expand Down
Loading

0 comments on commit b059247

Please sign in to comment.