Skip to content

Commit

Permalink
[ESLint] Suggest to destructure props when they are only used as memb…
Browse files Browse the repository at this point in the history
…ers (#14993)

* Suggest to destructure props when they are only used as members

* Add more tests

* Fix a bug
  • Loading branch information
gaearon authored Mar 1, 2019
1 parent 59ef284 commit e1e45fb
Show file tree
Hide file tree
Showing 2 changed files with 223 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -2224,7 +2224,189 @@ const tests = {
}
`,
errors: [
// TODO: make this message clearer since it's not obvious why.
"React Hook useEffect has a missing dependency: 'props'. " +
'Either include it or remove the dependency array. ' +
'Alternatively, destructure the necessary props outside the callback.',
],
},
{
code: `
function MyComponent(props) {
useEffect(() => {
function play() {
props.onPlay();
}
function pause() {
props.onPause();
}
}, []);
}
`,
output: `
function MyComponent(props) {
useEffect(() => {
function play() {
props.onPlay();
}
function pause() {
props.onPause();
}
}, [props]);
}
`,
errors: [
"React Hook useEffect has a missing dependency: 'props'. " +
'Either include it or remove the dependency array. ' +
'Alternatively, destructure the necessary props outside the callback.',
],
},
{
code: `
function MyComponent(props) {
useEffect(() => {
if (props.foo.onChange) {
props.foo.onChange();
}
}, []);
}
`,
output: `
function MyComponent(props) {
useEffect(() => {
if (props.foo.onChange) {
props.foo.onChange();
}
}, [props.foo]);
}
`,
errors: [
"React Hook useEffect has a missing dependency: 'props.foo'. " +
'Either include it or remove the dependency array.',
],
},
{
code: `
function MyComponent(props) {
useEffect(() => {
props.onChange();
if (props.foo.onChange) {
props.foo.onChange();
}
}, []);
}
`,
output: `
function MyComponent(props) {
useEffect(() => {
props.onChange();
if (props.foo.onChange) {
props.foo.onChange();
}
}, [props]);
}
`,
errors: [
"React Hook useEffect has a missing dependency: 'props'. " +
'Either include it or remove the dependency array. ' +
'Alternatively, destructure the necessary props outside the callback.',
],
},
{
code: `
function MyComponent(props) {
const [skillsCount] = useState();
useEffect(() => {
if (skillsCount === 0 && !props.isEditMode) {
props.toggleEditMode();
}
}, [skillsCount, props.isEditMode, props.toggleEditMode]);
}
`,
output: `
function MyComponent(props) {
const [skillsCount] = useState();
useEffect(() => {
if (skillsCount === 0 && !props.isEditMode) {
props.toggleEditMode();
}
}, [skillsCount, props.isEditMode, props.toggleEditMode, props]);
}
`,
errors: [
"React Hook useEffect has a missing dependency: 'props'. " +
'Either include it or remove the dependency array. ' +
'Alternatively, destructure the necessary props outside the callback.',
],
},
{
code: `
function MyComponent(props) {
const [skillsCount] = useState();
useEffect(() => {
if (skillsCount === 0 && !props.isEditMode) {
props.toggleEditMode();
}
}, []);
}
`,
output: `
function MyComponent(props) {
const [skillsCount] = useState();
useEffect(() => {
if (skillsCount === 0 && !props.isEditMode) {
props.toggleEditMode();
}
}, [props, skillsCount]);
}
`,
errors: [
"React Hook useEffect has missing dependencies: 'props' and 'skillsCount'. " +
'Either include them or remove the dependency array. ' +
'Alternatively, destructure the necessary props outside the callback.',
],
},
{
code: `
function MyComponent(props) {
useEffect(() => {
externalCall(props);
props.onChange();
}, []);
}
`,
output: `
function MyComponent(props) {
useEffect(() => {
externalCall(props);
props.onChange();
}, [props]);
}
`,
// Don't suggest to destructure props here since you can't.
errors: [
"React Hook useEffect has a missing dependency: 'props'. " +
'Either include it or remove the dependency array.',
],
},
{
code: `
function MyComponent(props) {
useEffect(() => {
props.onChange();
externalCall(props);
}, []);
}
`,
output: `
function MyComponent(props) {
useEffect(() => {
props.onChange();
externalCall(props);
}, [props]);
}
`,
// Don't suggest to destructure props here since you can't.
errors: [
"React Hook useEffect has a missing dependency: 'props'. " +
'Either include it or remove the dependency array.',
],
Expand Down
40 changes: 40 additions & 0 deletions packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,46 @@ export default {
}
}

// `props.foo()` marks `props` as a dependency because it has
// a `this` value. This warning can be confusing.
// So if we're going to show it, append a clarification.
if (!extraWarning && missingDependencies.has('props')) {
let propDep = dependencies.get('props');
if (propDep == null) {
return;
}
const refs = propDep.reference.resolved.references;
if (!Array.isArray(refs)) {
return;
}
let isPropsOnlyUsedInMembers = true;
for (let i = 0; i < refs.length; i++) {
const ref = refs[i];
const id = fastFindReferenceWithParent(
componentScope.block,
ref.identifier,
);
if (!id) {
isPropsOnlyUsedInMembers = false;
break;
}
const parent = id.parent;
if (parent == null) {
isPropsOnlyUsedInMembers = false;
break;
}
if (parent.type !== 'MemberExpression') {
isPropsOnlyUsedInMembers = false;
break;
}
}
if (isPropsOnlyUsedInMembers) {
extraWarning =
' Alternatively, destructure the necessary props ' +
'outside the callback.';
}
}

context.report({
node: declaredDependenciesNode,
message:
Expand Down

0 comments on commit e1e45fb

Please sign in to comment.