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.

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

[eslint] strip tailing property in assignments #16784

Merged
merged 2 commits into from
Apr 4, 2020

Conversation

Zzzen
Copy link
Contributor

@Zzzen Zzzen commented Sep 14, 2019

closes #15510

let foo = {}
useEffect(() => {
  foo.bar.baz = 43;
}, []);

This asks you to include foo.bar.baz into deps. But this doesn't make sense, as you write to it. Instead it should ask to include foo.bar into array.

@sizebot
Copy link

sizebot commented Sep 14, 2019

Details of bundled changes.

Comparing: 45b6443...5ded688

eslint-plugin-react-hooks

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
eslint-plugin-react-hooks.development.js +0.6% +0.4% 75.25 KB 75.68 KB 17.31 KB 17.38 KB NODE_DEV
eslint-plugin-react-hooks.production.min.js 🔺+0.5% 🔺+0.2% 20.14 KB 20.24 KB 6.9 KB 6.91 KB NODE_PROD

Generated by 🚫 dangerJS against 5ded688

@stale
Copy link

stale bot commented Jan 9, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 9, 2020
@necolas necolas requested a review from gaearon January 9, 2020 23:15
@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Jan 9, 2020
@necolas necolas added Resolution: Backlog Resolution: Stale Automatically closed due to inactivity labels Jan 9, 2020
@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Jan 9, 2020
@gaearon
Copy link
Collaborator

gaearon commented Apr 1, 2020

Can you please rebase? We moved to the suggestions API so the test needs tweaks.

@Zzzen
Copy link
Contributor Author

Zzzen commented Apr 2, 2020

👌 I'll try this weekend.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 2, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2381833:

Sandbox Source
great-proskuriakova-soe0l Configuration

@sizebot
Copy link

sizebot commented Apr 2, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 2381833

@sizebot
Copy link

sizebot commented Apr 2, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 2381833

@Zzzen
Copy link
Contributor Author

Zzzen commented Apr 2, 2020

Moved to the suggestions API. PTAL

@@ -1437,6 +1437,25 @@ function getDependency(node) {
)
) {
return getDependency(node.parent);
} else {
return stripTailingPropInAssignment(node);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What makes you think it's an assignment in this branch? Can we make it more specific and throw on something unexpected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😅 tryStrippingTrailingPropsInAssigment could have been a better choice. I'm trying to enumerate all cases and confused by a test:

function MyComponent(props) {
  useEffect(() => {
    if (props.foo.onChange) {
      props.foo.onChange();
    }
  }, []);
}

Why is props.foo a proper dependency? Why not props.foo.onChange?

* (props.foo.bar) = X => (props.foo).bar = X
* (props.foo.bar.baz) = X => (props.foo.bar).baz
*/
function stripTailingPropInAssignment(node) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just inline in this function into the parent one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌

@gaearon gaearon merged commit 2ff27ec into facebook:master Apr 4, 2020
@gaearon
Copy link
Collaborator

gaearon commented Apr 4, 2020

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ESLint] Assignment like foo.bar.baz = X should warn about foo.bar instead
5 participants