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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eslint: add hook rules #771

Merged
merged 5 commits into from May 14, 2019

Conversation

Projects
None yet
2 participants
@sohkai
Copy link
Member

commented May 8, 2019

Similar settings to those in the Voting app.

Was wondering why there were some many missed dependencies in the cached lists 馃槃.

Note that there are some unfixed rules in Preferences/LocalIdentities; for the sake of simplicity we can fix those after #768.

sohkai added some commits May 8, 2019

@sohkai sohkai requested review from bpierre and AquiGorka May 8, 2019

@@ -21,7 +21,7 @@ const ActivityPanel = React.memo(
if (open) {
frameRef.current.focus()
}
}, [frameRef.current, open])
}, [frameRef, open])

This comment has been minimized.

Copy link
@sohkai

sohkai May 8, 2019

Author Member

As it turns out, we're not supposed to use ref.current in the dependency list, but simply ref (e.g. https://reactjs.org/docs/hooks-faq.html#how-to-read-an-often-changing-value-from-usecallback)

This comment has been minimized.

Copy link
@bpierre

bpierre May 8, 2019

Member

TIL, thanks!

@bpierre

bpierre approved these changes May 8, 2019

Copy link
Member

left a comment

鉁旓笍 鉁旓笍 鉁旓笍


const prev = useCallback(() => {
if (step > 0) {
setStep(step - 1)
}
}, [step, steps])
}, [setStep, step])

return {

This comment has been minimized.

Copy link
@sohkai

sohkai May 8, 2019

Author Member

@bpierre Running into a caching problem with the useStep() hook. setStep() has a dependency on step, so it gets recreated whenever the step changes.

However, this becomes problematic in situations where we'd like to use setStep() in useEffect()s (e.g. the UpgradeModal, which causes it to be stuck on step 0).

It seems a bit wrong to ask users of the hook to always ignore setStep() from their dependency list :/

This comment has been minimized.

Copy link
@sohkai

sohkai May 8, 2019

Author Member

Tried to define a getStep() callback

const getStep = useCallback(() => step, [step])

But for some reason this never works when going back to the first step :/. It always caches step as 0 instead of 1.

This comment has been minimized.

Copy link
@bpierre

bpierre May 9, 2019

Member

I found two ways to solve this: by using a reducer, or by using a reference. It seems the reducer solution is the most 鈥渞eact-y way鈥 of doing it, so I opened a PR using it.

Re-reading this article after having played a bit with hooks was actually an eye opener about useReducer(): I was trying to avoid it until I had a case for something looking like a redux store, but I understand now how it can be useful for much smaller things, like in that case.

One thing I don鈥檛 really like is the way the steps is passed: the article recommends to declare the reducer in the render function to make it aware of props (or hook params), but it also says 鈥渢his pattern disables a few optimizations so try not to use it everywhere, but you can totally access props from a reducer if you need to鈥. I can imagine that the useReducer hook would have to update its reducer function every time we render, which might not be optimal depending on how it鈥檚 done internally. That鈥檚 why I opted for passing it in the action directly, which seems acceptable for such a small case.

This comment has been minimized.

Copy link
@sohkai

sohkai May 9, 2019

Author Member

This is why I like to think of useReducer as the 鈥渃heat mode鈥 of Hooks. It lets me decouple the update logic from describing what happened. This, in turn, helps me remove unnecessary dependencies from my effects and avoid re-running them more often than necessary.

This sounds like exactly what we were encountering 馃槃

bpierre and others added some commits May 9, 2019

@sohkai sohkai requested a review from bpierre May 13, 2019

@bpierre
Copy link
Member

left a comment

馃

@sohkai sohkai merged commit c4e077b into master May 14, 2019

7 checks passed

License Compliance All checks passed.
Details
build build
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
install install
Details
license/cla Contributor License Agreement is signed.
Details
lint lint
Details

@sohkai sohkai deleted the eslint-add-hook-rules branch May 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.