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.

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

[Logs UI] Remove apollo deps from log link-to routes #74502

Merged
merged 11 commits into from
Aug 14, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ export const logSourceConfigurationOriginRT = rt.keyof({
export type LogSourceConfigurationOrigin = rt.TypeOf<typeof logSourceConfigurationOriginRT>;

const logSourceFieldsConfigurationRT = rt.strict({
container: rt.string,
host: rt.string,
pod: rt.string,
timestamp: rt.string,
tiebreaker: rt.string,
});
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/infra/common/inventory_models/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ export const findInventoryModel = (type: InventoryItemType) => {
};

interface InventoryFields {
message: string[];
host: string;
pod: string;
container: string;
Expand Down
8 changes: 6 additions & 2 deletions x-pack/plugins/infra/public/components/loading_page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,14 @@ import { FlexPage } from './page';

interface LoadingPageProps {
message?: ReactNode;
'data-test-subj'?: string;
}

export const LoadingPage = ({ message }: LoadingPageProps) => (
<FlexPage>
export const LoadingPage = ({
message,
'data-test-subj': dataTestSubj = 'loadingPage',
}: LoadingPageProps) => (
<FlexPage data-test-subj={dataTestSubj}>
<EuiPageBody>
<EuiPageContent verticalPosition="center" horizontalPosition="center">
<EuiFlexGroup alignItems="center">
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/infra/public/components/page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,6 @@ export const PageContent = euiStyled.div`
`;

export const FlexPage = euiStyled(EuiPage)`
align-self: stretch;
flex: 1 0 0%;
`;
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { LoadingPage } from './loading_page';

export const SourceLoadingPage: React.FunctionComponent = () => (
<LoadingPage
data-test-subj="sourceLoadingPage"
message={
<FormattedMessage
id="xpack.infra.sourceLoadingPage.loadingDataSourcesMessage"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { LogSourceConfiguration, LogSourceStatus, useLogSource } from './log_source';

type CreateUseLogSource = (sourceConfiguration?: { sourceId?: string }) => typeof useLogSource;

const defaultSourceId = 'default';

export const createUninitializedUseLogSourceMock: CreateUseLogSource = ({
sourceId = defaultSourceId,
} = {}) => () => ({
derivedIndexPattern: {
fields: [],
title: 'unknown',
},
hasFailedLoadingSource: false,
hasFailedLoadingSourceStatus: false,
initialize: jest.fn(),
isLoading: false,
isLoadingSourceConfiguration: false,
isLoadingSourceStatus: false,
isUninitialized: true,
loadSource: jest.fn(),
loadSourceConfiguration: jest.fn(),
loadSourceFailureMessage: undefined,
loadSourceStatus: jest.fn(),
sourceConfiguration: undefined,
sourceId,
sourceStatus: undefined,
updateSourceConfiguration: jest.fn(),
});

export const createLoadingUseLogSourceMock: CreateUseLogSource = ({
sourceId = defaultSourceId,
} = {}) => (args) => ({
...createUninitializedUseLogSourceMock({ sourceId })(args),
isLoading: true,
isLoadingSourceConfiguration: true,
isLoadingSourceStatus: true,
});

export const createLoadedUseLogSourceMock: CreateUseLogSource = ({
sourceId = defaultSourceId,
} = {}) => (args) => ({
...createUninitializedUseLogSourceMock({ sourceId })(args),
sourceConfiguration: createBasicSourceConfiguration(sourceId),
sourceStatus: {
logIndexFields: [],
logIndexStatus: 'available',
},
});

export const createBasicSourceConfiguration = (sourceId: string): LogSourceConfiguration => ({
id: sourceId,
origin: 'stored',
configuration: {
description: `description for ${sourceId}`,
logAlias: 'LOG_INDICES',
logColumns: [],
fields: {
container: 'CONTAINER_FIELD',
host: 'HOST_FIELD',
pod: 'POD_FIELD',
tiebreaker: 'TIEBREAKER_FIELD',
timestamp: 'TIMESTAMP_FIELD',
},
name: sourceId,
},
});

export const createAvailableSourceStatus = (logIndexFields = []): LogSourceStatus => ({
logIndexFields,
logIndexStatus: 'available',
});
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@
*/

import createContainer from 'constate';
import { useState, useMemo, useCallback } from 'react';
import { useCallback, useMemo, useState } from 'react';
import { useMountedState } from 'react-use';
import { HttpSetup } from 'src/core/public';
import {
LogSourceConfiguration,
LogSourceStatus,
LogSourceConfigurationPropertiesPatch,
LogSourceConfigurationProperties,
LogSourceConfigurationPropertiesPatch,
LogSourceStatus,
} from '../../../../common/http_api/log_sources';
import { useTrackedPromise } from '../../../utils/use_tracked_promise';
import { callFetchLogSourceConfigurationAPI } from './api/fetch_log_source_configuration';
Expand All @@ -32,6 +33,7 @@ export const useLogSource = ({
sourceId: string;
fetch: HttpSetup['fetch'];
}) => {
const getIsMounted = useMountedState();
const [sourceConfiguration, setSourceConfiguration] = useState<
LogSourceConfiguration | undefined
>(undefined);
Expand All @@ -45,6 +47,10 @@ export const useLogSource = ({
return await callFetchLogSourceConfigurationAPI(sourceId, fetch);
},
onResolve: ({ data }) => {
if (!getIsMounted()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this fixes the race condition you mentioned in the description.

Could you help me understand what was the problem? I see that useMountedState is a wrapper around useEffect. I looked for usages of these promise creators and I couldn't find uses that weren't wrapped in useEffect already.

I worry that this might introduce a "gotcha" if consumers of this API need to call the promises before their component is mounted (i.e. in useLayoutEffect). If I understand this correctly, calling the promise will do nothing.

Copy link
Member Author

Choose a reason for hiding this comment

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

useMountedState returns a predicate that indicates whether the component is mounted. This becomes important when the component unmounts while the promise is pending. Then, when the promise is resolved, the subsequent setState will cause an exception. Therefore all asynchronously called setStates should be guarded by such a check. I'm still trying to catch up with all the occurrences of that mistake in this plugin, so this is a small step.

Copy link
Member Author

@weltenwort weltenwort Aug 14, 2020

Choose a reason for hiding this comment

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

I worry that this might introduce a "gotcha" if consumers of this API need to call the promises before their component is mounted (i.e. in useLayoutEffect). If I understand this correctly, calling the promise will do nothing.

As described above, the promise itself is not the problem. It resolving and thereby causing a setState call before mounting or after unmounting is.

return;
}

setSourceConfiguration(data);
},
},
Expand All @@ -58,6 +64,10 @@ export const useLogSource = ({
return await callPatchLogSourceConfigurationAPI(sourceId, patchedProperties, fetch);
},
onResolve: ({ data }) => {
if (!getIsMounted()) {
return;
}

setSourceConfiguration(data);
loadSourceStatus();
},
Expand All @@ -72,6 +82,10 @@ export const useLogSource = ({
return await callFetchLogSourceStatusAPI(sourceId, fetch);
},
onResolve: ({ data }) => {
if (!getIsMounted()) {
return;
}

setSourceStatus(data);
},
},
Expand Down
Loading