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

Jump to bottom

[ESLint] Suggest to destructure props when they are only used as members #14993

Merged
merged 3 commits into from
Mar 1, 2019
Merged

[ESLint] Suggest to destructure props when they are only used as members #14993

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f8b30f9
Suggest to destructure props when they are only used as members
gaearon Mar 1, 2019
75a5db9
Add more tests
gaearon Mar 1, 2019
9e795c8
Fix a bug
gaearon Mar 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
161 changes: 160 additions & 1 deletion packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2224,7 +2224,166 @@ const tests = {
}
`,
errors: [
// TODO: make this message clearer since it's not obvious why.
"React Hook useEffect has a missing dependency: 'props'. " +
Copy link
Contributor

Choose a reason for hiding this comment

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

This error message kinda surprises me when taking into account our recent discussion ( #14876 ) around props referential equality etc.

If props have "unstable" reference then IMHO you shouldnt ever suggest using it as hook's dependency 🤔 The latter advice sounds way more appropriate (destructuring)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

props is OK as an escape hatch when you just want the rule to shut up. It's still stable if you only set state, for example. Better than nothing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldnt the advice get reordered then? First propose destructuring and only latter propose using props directly.

You are calling it an escape hatch to shut up the rule - so it sounds to me that this is actually not something which you'd like to recommend.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're welcome to send a PR. :-) There are more worrying things I'm currently focused on than this particular message.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, might prepare one - was just validating my thinking so far :)

'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.',
],
Expand Down
37 changes: 37 additions & 0 deletions packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,43 @@ 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 = 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