Skip to content

Commit

Permalink
Add special message for ref.current
Browse files Browse the repository at this point in the history
  • Loading branch information
gaearon committed Feb 19, 2019
1 parent e0203cb commit f5aec9a
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 4 deletions.
Expand Up @@ -1263,12 +1263,43 @@ const tests = {
}, [ref1, ref2, props.someOtherRefs, props.color]);
}
`,
// TODO: special message for the ref case.
errors: [
"React Hook useEffect has missing dependencies: 'props.someOtherRefs' and 'props.color'. " +
'Either include them or remove the dependency array.',
],
},
{
code: `
function MyComponent(props) {
const ref1 = useRef();
const ref2 = useRef();
useEffect(() => {
ref1.current.focus();
console.log(ref2.current.textContent);
alert(props.someOtherRefs.current.innerHTML);
fetch(props.color);
}, [ref1.current, ref2.current, props.someOtherRefs, props.color]);
}
`,
output: `
function MyComponent(props) {
const ref1 = useRef();
const ref2 = useRef();
useEffect(() => {
ref1.current.focus();
console.log(ref2.current.textContent);
alert(props.someOtherRefs.current.innerHTML);
fetch(props.color);
}, [props.someOtherRefs, props.color, ref1, ref2]);
}
`,
errors: [
"React Hook useEffect has unnecessary dependencies: 'ref1.current' and 'ref2.current'. " +
'Either exclude them or remove the dependency array. ' +
"Mutable values like 'ref1.current' aren't valid dependencies " +
"because their mutation doesn't re-render the component.",
],
},
{
code: `
function MyComponent() {
Expand All @@ -1286,10 +1317,11 @@ const tests = {
}, [ref]);
}
`,
// TODO: special message for the ref case.
errors: [
"React Hook useEffect has an unnecessary dependency: 'ref.current'. " +
'Either exclude it or remove the dependency array.',
'Either exclude it or remove the dependency array. ' +
"Mutable values like 'ref.current' aren't valid dependencies " +
"because their mutation doesn't re-render the component.",
],
},
{
Expand Down
22 changes: 21 additions & 1 deletion packages/eslint-plugin-react-hooks/src/ReactiveDeps.js
Expand Up @@ -302,6 +302,7 @@ export default {
let duplicateDependencies = new Set();
let unnecessaryDependencies = new Set();
let missingDependencies = new Set();
let showRefCurrentWarning = false;

let actualDependencies = Array.from(dependencies.keys());

Expand Down Expand Up @@ -382,14 +383,33 @@ export default {
} or remove the dependency array.`
);
};
let extraWarning = '';
if (unnecessaryDependencies.size > 0) {
let badRef = null;
Array.from(unnecessaryDependencies.keys()).forEach(key => {
if (badRef !== null) {
return;
}
if (key.endsWith('.current')) {
badRef = key;
}
});
if (badRef !== null) {
extraWarning =
` Mutable values like '${badRef}' aren't valid dependencies ` +
"because their mutation doesn't re-render the component.";
}
}

context.report({
node: declaredDependenciesNode,
message:
`React Hook ${context.getSource(reactiveHook)} has ` +
// To avoid a long message, show the next actionable item.
(list(missingDependencies, 'a', 'missing', 'include') ||
list(unnecessaryDependencies, 'an', 'unnecessary', 'exclude') ||
list(duplicateDependencies, 'a', 'duplicate', 'omit')),
list(duplicateDependencies, 'a', 'duplicate', 'omit')) +
extraWarning,
fix(fixer) {
// TODO: consider keeping the comments?
return fixer.replaceText(
Expand Down

0 comments on commit f5aec9a

Please sign in to comment.