schiehll / react-alert

alerts for React
MIT License
608 stars 98 forks source link

useAlert triggering undesired component rendering... #150

Closed danielfroz closed 4 years ago

danielfroz commented 4 years ago

I am working with react-alert and identified a problem... useAlert() hook is causing undesired component re-rendering...

Here is the code snippet (example):

const api = useApi()
const alert = useAlert()
const [ message, setMessage ] = useState()

const load = useCallback(() => {
  api.get('/message')
    .then(message => setMessage(message))
    .catch(error => alert.error(error))
}, [ api, alert ])
useEffect(() => load() , [ load ])

Problem here is that every time an alert is fired [alert.info()...show(),etc...], useAlert()'s context is changed, resulting in call to load() callback.

I cloned the project, went to the code and fixed it... The fix is pretty simple... Here is my version of useAlert.js:

import { useContext, useMemo } from 'react'
import DefaultContext from './Context'

const useAlert = Context => {
  const alertContext = useContext(Context || DefaultContext)
  const alert = useMemo(() => {
    return alertContext.current
  }, [ alertContext ])
  return alert
}

export default useAlert

Can you please add the useMemo() to your code base and create release 7.0.1 ? Thanks!

schiehll commented 4 years ago

Hey @danielfroz!

Try something like:

const api = useApi()
const { error: alertError } = useAlert()
const [ message, setMessage ] = useState()

const load = useCallback(() => {
  api.get('/message')
    .then(message => setMessage(message))
    .catch(error => alertError(error))
}, [ api, alertError ])
useEffect(() => load() , [ load ])

And see if it works for you.

You can open a PR to add the useMemo too if you want.

danielfroz commented 4 years ago

I honestly prefer to fix useAlert() (project is too big to change all useAlert() on all UI components). Will do a PR (actually checking docs on how to do that). Thanks

schiehll commented 4 years ago

I honestly prefer to fix useAlert() (project is too big to change all useAlert() on all UI components). Will do a PR (actually checking docs on how to do that). Thanks

Thanks!

I don't think there's documentation for this, but you can just fork it, change the code and open a PR!

danielfroz commented 4 years ago

@schiehll. Done. Thanks for promptly reply. Abs

schiehll commented 4 years ago

Just published v7.0.1! Thanks for the PR.

huan086 commented 3 years ago

useAlert exposes the list of alerts. Doesn't this change cause the list not to be updated?

const { error: alertError } = useAlert()

should be the only correct way to prevent unnecessary re-renders.