Skip to content

chore(website): Enable react-hooks exhaustive deps rules #5663

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

Merged
merged 7 commits into from
Sep 28, 2022
Merged

chore(website): Enable react-hooks exhaustive deps rules #5663

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6250d99
enable rules
amorimr Sep 15, 2022
0c70dca
fix warnings
amorimr Sep 15, 2022
1e4239a
Merge remote-tracking branch 'upstream/main' into enable-react-exaust…
amorimr Sep 25, 2022
9996cd3
Merge remote-tracking branch 'upstream/main' into enable-react-exaust…
amorimr Sep 27, 2022
59b66ac
add missing dependencies
amorimr Sep 27, 2022
c025a31
fix infinite rerenders
amorimr Sep 28, 2022
507c7a5
fix expansion of tree items
amorimr Sep 28, 2022
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
1 change: 0 additions & 1 deletion packages/website/.eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ module.exports = {
'react/jsx-no-target-blank': 'off',
'react/no-unescaped-entities': 'off',
'@typescript-eslint/internal/prefer-ast-types-enum': 'off',
'react-hooks/exhaustive-deps': 'off', // TODO: enable it later
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sets the rules to the default behaviour ("warn").

},
settings: {
react: {
Expand Down
12 changes: 11 additions & 1 deletion packages/website/src/components/ASTViewerTS.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,17 @@ export default function ASTViewerTS({
['TypeFlags', typeFlags],
);
setModel(serialize(value, scopeSerializer));
}, [value, syntaxKind]);
}, [
value,
syntaxKind,
nodeFlags,
tokenFlags,
modifierFlags,
objectFlags,
symbolFlags,
flowFlags,
typeFlags,
]);

return (
<ASTViewer position={position} onSelectNode={onSelectNode} value={model} />
Expand Down
4 changes: 2 additions & 2 deletions packages/website/src/components/OptionsSelector.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ function OptionsSelectorContent({
.then(() => {
setCopyLink(true);
});
}, []);
}, [setCopyLink]);

const copyMarkdownToClipboard = useCallback(() => {
if (isLoading) {
Expand All @@ -60,7 +60,7 @@ function OptionsSelectorContent({
void navigator.clipboard.writeText(createMarkdown(state)).then(() => {
setCopyMarkdown(true);
});
}, [state, isLoading]);
}, [isLoading, state, setCopyMarkdown]);

const openIssue = useCallback(() => {
if (isLoading) {
Expand Down
15 changes: 10 additions & 5 deletions packages/website/src/components/Playground.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,15 @@ function Playground(): JSX.Element {
[setState],
);

const onLoaded = useCallback(
(ruleNames: RuleDetails[], tsVersions: readonly string[]): void => {
setRuleNames(ruleNames);
setTSVersion(tsVersions);
setIsLoading(false);
},
[],
);

Comment on lines +87 to +95
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This small refactoring was necessary to fix an issue raised by updating one of the dependency arrays on useSandboxServices.ts:

Since the original onLoaded was inlined, it was being recreated at every render, causing racing conditions on useSandboxServices effects that crashed the page.

return (
<div className={styles.codeContainer}>
{ruleNames.length > 0 && (
Expand Down Expand Up @@ -150,11 +159,7 @@ function Playground(): JSX.Element {
onMarkersChange={setMarkers}
decoration={selectedRange}
onChange={setState}
onLoaded={(ruleNames, tsVersions): void => {
setRuleNames(ruleNames);
setTSVersion(tsVersions);
setIsLoading(false);
}}
onLoaded={onLoaded}
onSelect={setPosition}
/>
</div>
Expand Down
6 changes: 3 additions & 3 deletions packages/website/src/components/ast/Elements.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export function ComplexItem({
}
}
},
[data],
[data.model.range, onSelectNode],
);

useEffect(() => {
Expand All @@ -44,10 +44,10 @@ export function ComplexItem({
level !== 'ast' && selected && !hasChildInRange(selection, data.model),
);

if (selected && !isExpanded) {
if (selected) {
setIsExpanded(selected);
}
}, [selection, data]);
}, [selection, data, level]);

return (
<ItemGroup
Expand Down
14 changes: 8 additions & 6 deletions packages/website/src/components/ast/PropertyName.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,23 @@ export interface PropertyNameProps {
}

export default function PropertyName(props: PropertyNameProps): JSX.Element {
const { onClick: onClickProps, onHover } = props;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Destructuring props to avoid stale values as suggested by: facebook/react#16265 (comment)


const onClick = useCallback(
(e: MouseEvent<HTMLElement>) => {
e.preventDefault();
props.onClick?.(e);
onClickProps?.(e);
},
[props.onClick],
[onClickProps],
);

const onMouseEnter = useCallback(() => {
props.onHover?.(true);
}, [props.onHover]);
onHover?.(true);
}, [onHover]);

const onMouseLeave = useCallback(() => {
props.onHover?.(false);
}, [props.onHover]);
onHover?.(false);
}, [onHover]);

return props.onClick || props.onHover ? (
<>
Expand Down
2 changes: 1 addition & 1 deletion packages/website/src/components/ast/SimpleItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export function SimpleItem({
onSelectNode(state ? data.model.range : null);
}
},
[data],
[data.model.range, onSelectNode],
);

return (
Expand Down
15 changes: 8 additions & 7 deletions packages/website/src/components/config/ConfigEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,26 +89,27 @@ function isDefault(value: unknown, defaults?: unknown[]): boolean {
}

function ConfigEditor(props: ConfigEditorProps): JSX.Element {
const { onClose: onCloseProps, isOpen, values } = props;
const [filter, setFilter] = useState<string>('');
const [config, setConfig] = useReducer(reducerObject, {});
const [filterInput, setFilterFocus] = useFocus();

const onClose = useCallback(() => {
props.onClose(config);
}, [props.onClose, config]);
onCloseProps(config);
}, [onCloseProps, config]);

useEffect(() => {
setConfig({ type: 'init', config: props.values });
}, [props.values]);
setConfig({ type: 'init', config: values });
}, [values]);

useEffect(() => {
if (props.isOpen) {
if (isOpen) {
setFilterFocus();
}
}, [props.isOpen]);
}, [isOpen, setFilterFocus]);

return (
<Modal header={props.header} isOpen={props.isOpen} onClose={onClose}>
<Modal header={props.header} isOpen={isOpen} onClose={onClose}>
<div className={styles.searchBar}>
<Text
ref={filterInput}
Expand Down
21 changes: 11 additions & 10 deletions packages/website/src/components/config/ConfigEslint.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,20 +28,21 @@ function checkOptions(rule: [string, unknown]): rule is [string, RuleEntry] {
}

function ConfigEslint(props: ConfigEslintProps): JSX.Element {
const { isOpen, config, onClose: onCloseProps, ruleOptions } = props;
const [options, updateOptions] = useState<ConfigOptionsType[]>([]);
const [configObject, updateConfigObject] = useState<EslintRC>();

useEffect(() => {
if (props.isOpen) {
updateConfigObject(parseESLintRC(props.config));
if (isOpen) {
updateConfigObject(parseESLintRC(config));
}
}, [props.isOpen, props.config]);
}, [isOpen, config]);

useEffect(() => {
updateOptions([
{
heading: 'Rules',
fields: props.ruleOptions
fields: ruleOptions
.filter(item => item.name.startsWith('@typescript'))
.map(item => ({
key: item.name,
Expand All @@ -52,7 +53,7 @@ function ConfigEslint(props: ConfigEslintProps): JSX.Element {
},
{
heading: 'Core rules',
fields: props.ruleOptions
fields: ruleOptions
.filter(item => !item.name.startsWith('@typescript'))
.map(item => ({
key: item.name,
Expand All @@ -62,7 +63,7 @@ function ConfigEslint(props: ConfigEslintProps): JSX.Element {
})),
},
]);
}, [props.ruleOptions]);
}, [ruleOptions]);

const onClose = useCallback(
(newConfig: Record<string, unknown>) => {
Expand All @@ -76,22 +77,22 @@ function ConfigEslint(props: ConfigEslintProps): JSX.Element {
.filter(checkOptions),
);
if (!shallowEqual(cfg, configObject?.rules)) {
props.onClose({
onCloseProps({
eslintrc: toJson({ ...(configObject ?? {}), rules: cfg }),
});
} else {
props.onClose();
onCloseProps();
}
},
[props.onClose, configObject],
[onCloseProps, configObject],
);

return (
<ConfigEditor
header="Eslint Config"
options={options}
values={configObject?.rules ?? {}}
isOpen={props.isOpen}
isOpen={isOpen}
onClose={onClose}
/>
);
Expand Down
17 changes: 9 additions & 8 deletions packages/website/src/components/config/ConfigTypeScript.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,15 @@ interface ConfigTypeScriptProps {
}

function ConfigTypeScript(props: ConfigTypeScriptProps): JSX.Element {
const { onClose: onCloseProps, isOpen, config } = props;
const [tsConfigOptions, updateOptions] = useState<ConfigOptionsType[]>([]);
const [configObject, updateConfigObject] = useState<TSConfig>();

useEffect(() => {
if (props.isOpen) {
updateConfigObject(parseTSConfig(props.config));
if (isOpen) {
updateConfigObject(parseTSConfig(config));
}
}, [props.isOpen, props.config]);
}, [isOpen, config]);

useEffect(() => {
if (window.ts) {
Expand Down Expand Up @@ -54,28 +55,28 @@ function ConfigTypeScript(props: ConfigTypeScriptProps): JSX.Element {
),
);
}
}, [props.isOpen]);
}, [isOpen]);

const onClose = useCallback(
(newConfig: Record<string, unknown>) => {
const cfg = { ...newConfig };
if (!shallowEqual(cfg, configObject?.compilerOptions)) {
props.onClose({
onCloseProps({
tsconfig: toJson({ ...(configObject ?? {}), compilerOptions: cfg }),
});
} else {
props.onClose();
onCloseProps();
}
},
[props.onClose, configObject],
[onCloseProps, configObject],
);

return (
<ConfigEditor
header="TypeScript Config"
options={tsConfigOptions}
values={configObject?.compilerOptions ?? {}}
isOpen={props.isOpen}
isOpen={isOpen}
onClose={onClose}
/>
);
Expand Down
Loading