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
Merged
Changes from 2 commits
Commits
File filter...
Filter file types
Jump to鈥
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -19,12 +19,14 @@
},
"sourceType": "module"
},
"plugins": ["prettier", "react", "import", "promise"],
"plugins": ["prettier", "react", "react-hooks", "import", "promise"],
"rules": {
"react/prop-types": 'warn',
"react-hooks/rules-of-hooks": "error",
"react-hooks/exhaustive-deps": "warn",
"import/no-unresolved": ["error", { ignore: ["^react(-dom)?$", "^styled-components$"] }],
"promise/no-nesting": ["off"],
"valid-jsdoc": "error",
"react/prop-types": 'warn',
"linebreak-style": ["error", "unix"],
},
"settings": {
@@ -69,6 +69,7 @@
"eslint-plugin-prettier": "^2.7.0",
"eslint-plugin-promise": "^4.0.1",
"eslint-plugin-react": "^7.5.1",
"eslint-plugin-react-hooks": "^1.6.0",
"eslint-plugin-standard": "^4.0.0",
"husky": "^1.0.1",
"lint-staged": "^8.1.1",
@@ -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!


const handleBlur = useCallback(
event => {
@@ -1,4 +1,4 @@
import React from 'react'
import React, { useCallback, useEffect } from 'react'
import PropTypes from 'prop-types'
import { LocalIdentityModalContext } from '../LocalIdentityModal/LocalIdentityModalManager'
import { isAddress } from '../../web3-utils'
@@ -11,22 +11,20 @@ import LocalIdentityPopoverTitle from './LocalIdentityPopoverTitle'

const LocalIdentityBadge = ({ entity, ...props }) => {
const address = isAddress(entity) ? entity : null
if (address === null) {
return <IdentityBadgeWithNetwork {...props} customLabel={entity} />
}

const { resolve, identityEvents$ } = React.useContext(IdentityContext)
const { showLocalIdentityModal } = React.useContext(LocalIdentityModalContext)
const [label, setLabel] = React.useState(null)
const handleResolve = async () => {
const handleResolve = useCallback(async () => {
try {
const { name = null } = await resolve(address)
setLabel(name)
} catch (e) {
// address does not resolve to identity
}
}
const handleClick = () => {
}, [address, resolve, setLabel])

const handleClick = useCallback(() => {
showLocalIdentityModal(address)
.then(handleResolve)
.then(() =>
@@ -35,16 +33,22 @@ const LocalIdentityBadge = ({ entity, ...props }) => {
.catch(e => {
/* user cancelled modify intent */
})
}
const handleEvent = updatedAddress => {
if (updatedAddress.toLowerCase() === address.toLowerCase()) {
handleResolve()
}
}
const clearLabel = () => {
}, [address, identityEvents$, handleResolve, showLocalIdentityModal])

const handleEvent = useCallback(
updatedAddress => {
if (updatedAddress.toLowerCase() === address.toLowerCase()) {
handleResolve()
}
},
[address, handleResolve]
)

const clearLabel = useCallback(() => {
setLabel(null)
}
React.useEffect(() => {
}, [setLabel])

useEffect(() => {
handleResolve()
const subscription = identityEvents$.subscribe(event => {
switch (event.type) {
@@ -59,7 +63,11 @@ const LocalIdentityBadge = ({ entity, ...props }) => {
return () => {
subscription.unsubscribe()
}
}, [entity, identityEvents$])
}, [clearLabel, entity, handleEvent, handleResolve, identityEvents$])

if (address === null) {
return <IdentityBadgeWithNetwork {...props} customLabel={entity} />
}

return (
<IdentityBadgeWithNetwork
@@ -1,4 +1,4 @@
import React from 'react'
import React, { useCallback, useEffect } from 'react'
import PropTypes from 'prop-types'
import styled from 'styled-components'
import { Button, TextInput, breakpoint, font, theme } from '@aragon/ui'
@@ -11,7 +11,7 @@ const LocalIdentityModal = ({ opened, ...props }) => {
const { showModal, hideModal } = React.useContext(ModalContext)
React.useEffect(() => {
opened ? showModal(Modal, props) : hideModal()
}, [opened])
}, [opened, showModal, hideModal, props])

return null
}
@@ -24,10 +24,12 @@ const Modal = ({ address, label, onCancel, onSave }) => {
const [action, setAction] = React.useState(null)
const [error, setError] = React.useState(null)
const labelInput = React.useRef(null)
const handleCancel = () => {

const handleCancel = useCallback(() => {
onCancel()
}
const handleSave = () => {
}, [onCancel])

const handleSave = useCallback(() => {
try {
const label = labelInput.current.value.trim()
if (label) {
@@ -36,21 +38,26 @@ const Modal = ({ address, label, onCancel, onSave }) => {
} catch (e) {
setError(e)
}
}
const handlekeyDown = e => {
if (e.keyCode === keycodes.enter) {
handleSave()
} else if (e.keyCode === keycodes.esc) {
handleCancel()
}
}
React.useEffect(() => {
}, [address, labelInput, onSave, setError])

const handleKeyDown = useCallback(
e => {
if (e.keyCode === keycodes.enter) {
handleSave()
} else if (e.keyCode === keycodes.esc) {
handleCancel()
}
},
[handleCancel, handleSave]
)

useEffect(() => {
setAction(label && label.trim() ? 'Edit' : 'Add')
labelInput.current.focus()
labelInput.current.select()
window.addEventListener('keydown', handlekeyDown)
return () => window.removeEventListener('keydown', handlekeyDown)
}, [])
window.addEventListener('keydown', handleKeyDown)
return () => window.removeEventListener('keydown', handleKeyDown)
}, [label, labelInput, handleKeyDown, setAction])

return (
<EscapeOutside onEscapeOutside={onCancel}>
@@ -1,4 +1,4 @@
import React from 'react'
import React, { useCallback, useEffect } from 'react'
import PropTypes from 'prop-types'
import styled from 'styled-components'
import { Transition, animated } from 'react-spring'
@@ -28,47 +28,62 @@ const Preferences = ({ dao, onClose, smallView, wrapper }) => {
const { identityEvents$ } = React.useContext(IdentityContext)
const [selectedTab, setSelectedTab] = React.useState(0)
const [localIdentities, setLocalIdentities] = React.useState({})
const handleGetAll = async () => {

const handleGetAll = useCallback(async () => {
if (!wrapper) {
return
}
setLocalIdentities(await wrapper.getLocalIdentities())
}
const handleClearAll = async () => {
}, [setLocalIdentities, wrapper])

const handleClearAll = useCallback(async () => {
if (!wrapper) {
return
}
await wrapper.clearLocalIdentities()
setLocalIdentities({})
identityEvents$.next({ type: identityEventTypes.CLEAR })
}
const handleModify = (address, data) => {
if (!wrapper) {
return
}
wrapper.modifyAddressIdentity(address, data)
}
const handleImport = async list => {
if (!wrapper) {
return
}
setLocalIdentities({})
for (const { name, address } of list) {
await wrapper.modifyAddressIdentity(address, { name })
}
setLocalIdentities(await wrapper.getLocalIdentities())
identityEvents$.next({ type: identityEventTypes.IMPORT })
}
const handlekeyDown = e => {
if (e.keyCode === keycodes.esc) {
onClose()
}
}
React.useEffect(() => {
}, [identityEvents$, wrapper])

const handleModify = useCallback(
(address, data) => {
if (!wrapper) {
return
}
wrapper.modifyAddressIdentity(address, data)
},
[wrapper]
)

const handleImport = useCallback(
async list => {
if (!wrapper) {
return
}
setLocalIdentities({})
for (const { name, address } of list) {
await wrapper.modifyAddressIdentity(address, { name })
}
setLocalIdentities(await wrapper.getLocalIdentities())
identityEvents$.next({ type: identityEventTypes.IMPORT })
},
[identityEvents$, setLocalIdentities, wrapper]
)

const handlekeyDown = useCallback(
e => {
if (e.keyCode === keycodes.esc) {
onClose()
}
},
[onClose]
)

useEffect(() => {
handleGetAll()
window.addEventListener('keydown', handlekeyDown)
return () => window.removeEventListener('keydown', handlekeyDown)
}, [])
}, [handleGetAll, handlekeyDown])

return (
<AppView
@@ -21,7 +21,7 @@ const UpgradeModal = React.memo(({ visible, onUpgrade, onClose }) => {
if (visible) {
setStep(0)
}
}, [visible])
}, [setStep, visible])

return (
<Viewport>
@@ -86,7 +86,7 @@ const UpgradeOrganizationPanel = React.memo(
// Or, the user just can't perform this action.
await wrapper.performTransactionPath(upgradePath.path)
}
}, [repos, wrapper])
}, [daoAddress, onClose, repos, wrapper])

return (
<SidePanel
@@ -60,13 +60,13 @@ export function useSteps(steps) {
if (step < steps - 1) {
setStep(step + 1)
}
}, [step, steps])
}, [setStep, step, steps])

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 馃槃

direction,
ProTip! Use n and p to navigate between commits in a pull request.
You can鈥檛 perform that action at this time.