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

[es_ui_shared] Fix eslint exhaustive deps rule #76392

Merged
merged 11 commits into from
Sep 3, 2020
Merged

[es_ui_shared] Fix eslint exhaustive deps rule #76392

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e0d7039
Fix GlobalFlyout rule
sebelga Sep 1, 2020
7e7611a
Fix useJson and <JsonEditor /> rule
sebelga Sep 1, 2020
6d152dd
Fix <RangeField /> rule
sebelga Sep 1, 2020
c09a5be
Merge branch 'master' into fix-es-lint-exhaustive-deps-rule
elasticmachine Sep 2, 2020
8727c9d
Merge branch 'master' into fix-es-lint-exhaustive-deps-rule
elasticmachine Sep 2, 2020
bba9d40
Fix JSON editor when it is "controlled"
sebelga Sep 3, 2020
1f9ae41
Update embeddable InputEditor example to use non controlled JsonEditor
sebelga Sep 3, 2020
ba97afd
Merge branch 'fix-es-lint-exhaustive-deps-rule' of github.com:sebelga…
sebelga Sep 3, 2020
438d9f2
Revert "Update embeddable InputEditor example to use non controlled J…
sebelga Sep 3, 2020
16a8514
Fix JsonEditor: don't use hook state when component is controlled
sebelga Sep 3, 2020
8f2c42f
Merge remote-tracking branch 'upstream/master' into fix-es-lint-exhau…
sebelga Sep 3, 2020
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
4 changes: 1 addition & 3 deletions src/plugins/es_ui_shared/__packages_do_not_import__/global_flyout/global_flyout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,9 +160,7 @@ export const useGlobalFlyout = () => {
Array.from(getContents()).forEach(removeContent);
}
};
// https://github.com/elastic/kibana/issues/73970
/* eslint-disable-next-line react-hooks/exhaustive-deps */
}, [removeContent]);
}, [removeContent, getContents]);

return { ...ctx, addContent };
};
11 changes: 5 additions & 6 deletions src/plugins/es_ui_shared/public/components/json_editor/json_editor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/

import React, { useCallback } from 'react';
import React, { useCallback, useMemo } from 'react';
import { EuiFormRow, EuiCodeEditor } from '@elastic/eui';
import { debounce } from 'lodash';

Expand Down Expand Up @@ -52,9 +52,9 @@ export const JsonEditor = React.memo(
isControlled,
});

// https://github.com/elastic/kibana/issues/73971
/* eslint-disable-next-line react-hooks/exhaustive-deps */
const debouncedSetContent = useCallback(debounce(setContent, 300), [setContent]);
const debouncedSetContent = useMemo(() => {
return debounce(setContent, 300);
}, [setContent]);

// We let the consumer control the validation and the error message.
const error = isControlled ? propsError : internalError;
Expand All @@ -78,8 +78,7 @@ export const JsonEditor = React.memo(
debouncedSetContent(updated);
}
},
/* eslint-disable-next-line react-hooks/exhaustive-deps */
[isControlled]
[isControlled, onUpdate, debouncedSetContent]
);

return (
Expand Down
49 changes: 28 additions & 21 deletions src/plugins/es_ui_shared/public/components/json_editor/use_json.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/

import { useEffect, useState, useRef } from 'react';
import { useEffect, useState, useRef, useCallback } from 'react';
import { i18n } from '@kbn/i18n';

import { isJSON } from '../../../static/validators/string';
Expand Down Expand Up @@ -45,11 +45,11 @@ export const useJson = <T extends object = { [key: string]: any }>({
onUpdate,
isControlled = false,
}: Parameters<T>) => {
const didMount = useRef(false);
const isMounted = useRef(false);
const [content, setContent] = useState<string>(stringifyJson(defaultValue));
const [error, setError] = useState<string | null>(null);

const validate = () => {
const validate = useCallback(() => {
// We allow empty string as it will be converted to "{}""
const isValid = content.trim() === '' ? true : isJSON(content);
if (!isValid) {
Expand All @@ -62,31 +62,38 @@ export const useJson = <T extends object = { [key: string]: any }>({
setError(null);
}
return isValid;
};
}, [content]);

const formatContent = () => {
const formatContent = useCallback(() => {
const isValid = validate();
const data = isValid && content.trim() !== '' ? JSON.parse(content) : {};
return data as T;
};
}, [validate, content]);

useEffect(() => {
if (didMount.current) {
const isValid = isControlled ? undefined : validate();
onUpdate({
data: {
raw: content,
format: formatContent,
},
validate,
isValid,
});
} else {
didMount.current = true;
if (!isMounted.current) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having trouble figuring out how we'd get into this state. Did you run into a situation where the component wasn't mounted when this function was called?

Copy link
Contributor Author

@sebelga sebelga Sep 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Lol that's a great catch! 😊 I refactored a bit too quickly, I will put back how it was before. The intention was to not call the onUpdate on mount as... nothing has been updated yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EDIT: Looking at the code it is correct. So like I said, the idea is not to call the onUpdate on component mount as there isn't any update.

In the next useEffect the component isMounted.current will be set to true so on the next update it will call the handler.

Copy link
Contributor

@cjcenizal cjcenizal Sep 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, I see. I think the use of the name isMounted is confusing to me, particularly because it defaults to false. The first time a hook is called is when the owner mounts, so by definition when I'm walking through a hook's "lifecycle" I begin with the mindset of it being mounted somewhere. To fix this, how about renaming the variable isInitialCall or isFirstUpdate, defaulting it to true and then setting it to false? I think this area of the code also deserves a comment explaining your intentions to the reader.

Copy link
Contributor Author

@sebelga sebelga Sep 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

particularly because it defaults to false

I must default to false. when the ref is created the component has not mounted yet. It only has mounted when a useEffect is called (every useEffect is called in order of appearance in the code. I learned that the hard way).

So the convention I have put in place everywhere is the same: isMounted (because that is the React term for this and I prefer not to add another concept) defaults to false and is set to true in the last useEffect in the code. So any previous useEffect will be called when mounting but the ref will still be false for them. This is exactly what we want: have a ref in our useEffects telling us if the component has mounted or not, and have some conditional logic to prevent doing certain things "on component mount". As you commented somewhere, useEffect should have a single purpose and not one giant useEffect (which would probably not work anyway).

This is what I shared in my "Lesson learned" here #75796

You will see around things like this

const didMount = useRef(false);

useEffect(() => {
  if (didMount.current) {
    // do some logic **after** the initial component mount
  } else {
    // the component has just mounted, nothing to do at that lifecycle
    didMount.current = true;
  }
}, [<deps that will influence the useEffect and get us ready for hours of debugging>]);

My convention is exactly this pattern but, as I have learned, it is much better to have a dedicated useEffect to handle this ref, and set it as last useEffect without any deps.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Thanks for explaining. I think some comments in the code would help explain this to others (or even our future selves).

return;
}
// https://github.com/elastic/kibana/issues/73971
/* eslint-disable-next-line react-hooks/exhaustive-deps */
}, [content]);

const isValid = isControlled ? undefined : validate();

onUpdate({
data: {
raw: content,
format: formatContent,
},
validate,
isValid,
});
}, [onUpdate, content, isControlled, formatContent, validate]);

useEffect(() => {
isMounted.current = true;

return () => {
isMounted.current = false;
};
}, []);

return {
content,
Expand Down
7 changes: 3 additions & 4 deletions src/plugins/es_ui_shared/static/forms/components/fields/range_field.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,17 +31,16 @@ interface Props {

export const RangeField = ({ field, euiFieldProps = {}, ...rest }: Props) => {
const { isInvalid, errorMessage } = getFieldValidityAndErrorMessage(field);
const { onChange: onFieldChange } = field;

const onChange = useCallback(
(e: React.ChangeEvent<HTMLInputElement> | React.MouseEvent<HTMLButtonElement>) => {
const event = ({ ...e, value: `${e.currentTarget.value}` } as unknown) as React.ChangeEvent<{
value: string;
}>;
field.onChange(event);
onFieldChange(event);
},
// https://github.com/elastic/kibana/issues/73972
/* eslint-disable-next-line react-hooks/exhaustive-deps */
[field.onChange]
[onFieldChange]
);

return (
Expand Down
6 changes: 3 additions & 3 deletions ...pplication/components/mappings_editor/components/load_mappings/load_mappings_provider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import React, { useState, useRef } from 'react';
import React, { useState, useRef, useCallback } from 'react';
import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n/react';
import {
Expand Down Expand Up @@ -134,9 +134,9 @@ export const LoadMappingsProvider = ({ onJson, children }: Props) => {
state.json !== undefined && state.errors !== undefined ? 'validationResult' : 'json';
const i18nTexts = getTexts(view, state.errors?.length);

const onJsonUpdate: OnJsonEditorUpdateHandler = (jsonUpdateData) => {
const onJsonUpdate: OnJsonEditorUpdateHandler = useCallback((jsonUpdateData) => {
jsonContent.current = jsonUpdateData;
};
}, []);

const openModal: OpenJsonModalFunc = () => {
setState({ isModalOpen: true });
Expand Down
8 changes: 5 additions & 3 deletions ...cation/components/pipeline_processors_editor/components/load_from_json/modal_provider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n/react';
import React, { FunctionComponent, useRef, useState } from 'react';
import React, { FunctionComponent, useRef, useState, useCallback } from 'react';
import { EuiConfirmModal, EuiOverlayMask, EuiSpacer, EuiText, EuiCallOut } from '@elastic/eui';

import { JsonEditor, OnJsonEditorUpdateHandler } from '../../../../../shared_imports';
Expand Down Expand Up @@ -66,10 +66,12 @@ export const ModalProvider: FunctionComponent<Props> = ({ onDone, children }) =>
raw: defaultValueRaw,
},
});
const onJsonUpdate: OnJsonEditorUpdateHandler = (jsonUpdateData) => {

const onJsonUpdate: OnJsonEditorUpdateHandler = useCallback((jsonUpdateData) => {
setIsValidJson(jsonUpdateData.validate());
jsonContent.current = jsonUpdateData;
};
}, []);

return (
<>
{children(() => setIsModalVisible(true))}
Expand Down