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] Tweak setState updater message and add useEffect(async) warning #15055

Merged
merged 2 commits into from
Mar 7, 2019

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Mar 7, 2019

What it says.

In-person testing showed using original variable name confuses people.
@@ -2304,7 +2327,7 @@ const tests = {
errors: [
"React Hook useEffect has a missing dependency: 'state'. " +
'Either include it or remove the dependency array. ' +
`You can also write 'setState(state => ...)' ` +
`You can also write 'setState(s => ...)' ` +
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you decide to do a single character here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Showed it to people and they were confused seeing the same variable name because it wasn't obvious it's being shadowed. This explanation clicked tho.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess I'm open to other changes, just want to avoid the shadowing. If people think this is confusing we can brainstorm more options.

Copy link
Contributor

Choose a reason for hiding this comment

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

No I trust your judgement, I was just curious. It makes sense. Thanks!
(ashamed to say I usually use x =>..., which people around me hate)

@gaearon gaearon merged commit 03ad9c7 into facebook:master Mar 7, 2019
@gaearon gaearon deleted the more-lint-twks branch March 7, 2019 19:41
@sizebot
Copy link

sizebot commented Mar 7, 2019

Details of bundled changes.

Comparing: eb6247a...ee45567

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.8% +1.1% 72.45 KB 73.01 KB 16.53 KB 16.7 KB NODE_DEV
eslint-plugin-react-hooks.production.min.js 🔺+2.2% 🔺+3.5% 18.88 KB 19.3 KB 6.43 KB 6.65 KB NODE_PROD
ESLintPluginReactHooks-dev.js +1.0% +1.1% 77.43 KB 78.17 KB 16.97 KB 17.17 KB FB_WWW_DEV

Generated by 🚫 dangerJS

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.

None yet

4 participants