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

remove warning for use(Layout)Effect #2358

Merged
merged 2 commits into from
Feb 18, 2020
Merged

Conversation

JoviDeCroock
Copy link
Member

@JoviDeCroock JoviDeCroock commented Feb 18, 2020

Due to a conversation on Slack, which rightfully pointed out that libraries often use this for animations, ... I wanted to put this PR up for discussion. We should refer to eslint-plugin-hooks instead of implementing this ourselves. eslint will ensure we only evaluate user code

Another good argument for removing it: https://github.com/preactjs/preact/blob/master/hooks/src/index.js#L184

@github-actions
Copy link

github-actions bot commented Feb 18, 2020

Size Change: -405 B (1%)

Total Size: 38.1 kB

Filename Size Change
debug/dist/debug.js 2.95 kB -134 B (4%)
debug/dist/debug.module.js 2.93 kB -135 B (4%)
debug/dist/debug.umd.js 3.01 kB -136 B (4%)
ℹ️ View Unchanged
Filename Size Change
compat/dist/compat.js 3 kB 0 B
compat/dist/compat.module.js 3.03 kB 0 B
compat/dist/compat.umd.js 3.05 kB 0 B
devtools/dist/devtools.js 175 B 0 B
devtools/dist/devtools.module.js 185 B 0 B
devtools/dist/devtools.umd.js 252 B 0 B
dist/preact.js 3.73 kB 0 B
dist/preact.min.js 3.73 kB 0 B
dist/preact.module.js 3.75 kB 0 B
dist/preact.umd.js 3.79 kB 0 B
hooks/dist/hooks.js 1.05 kB 0 B
hooks/dist/hooks.module.js 1.08 kB 0 B
hooks/dist/hooks.umd.js 1.13 kB 0 B
test-utils/dist/testUtils.js 390 B 0 B
test-utils/dist/testUtils.module.js 392 B 0 B
test-utils/dist/testUtils.umd.js 469 B 0 B

compressed-size-action

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.004%) to 99.789% when pulling 4a65ac1 on remove-effect-warnings into 4a073c8 on master.

@xiel
Copy link
Contributor

xiel commented Feb 18, 2020

Thanks for your quick reaction and the PR. Just re-posting my PR comment here:


There are valid use cases where useEffect and useLayoutEffect must be used without a dependency array. As these warnings are not configurable, they are a massive deterioration of the developer experience as it makes the console really messy.

In my option there shouldn't be (non-configurable) warnings while using the API as designed.
The dependency array is described as optional for all useEffect, useLayoutEffect and useImperativeHandle in the react docs.
https://reactjs.org/docs/hooks-reference.html#useeffect

eg. my console gets flooded with 100s of warnings like:

You should provide an array of arguments as the second argument to the "useLayoutEffect" hook.
Not doing so will invoke this effect on every render.

Currently using useImperativeHandle can also trigger a misleading useLayoutEffect warning, as it accepts an empty deps array as well and uses useLayoutEffect in its implementation
https://github.com/preactjs/preact/blob/master/hooks/src/index.js#L178

Examples:

  • using refs after render to measure, manipulate focus, animate
  • read and write refs in effects
  • sync dom nodes with external libs
  • eg. see popular libraries like react-spring (useSpring)

These cases are better covered by the react hooks eslint plugin:
https://www.npmjs.com/package/eslint-plugin-react-hooks#installation

@JoviDeCroock
Copy link
Member Author

@xiel thanks for the detailed explanation and my apologies for not reporting back on Slack, I completely forgot to check back.

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

Successfully merging this pull request may close these issues.

5 participants