search

mifi / react-lottie-player

Fully declarative React Lottie player
MIT License
495 stars 52 forks source link

reduce usage of useEffect #74

Closed Kepro closed 2 years ago

Kepro commented 2 years ago

just removed half of useEffects

mifi commented 2 years ago

have you tested this code? I believe there's a reason (as per the comment you removed) that the event listeners need to be removed before destroy is called https://github.com/mifi/react-lottie-player/blob/d975f933fe3f166460329c9f79c7af29df82d2cc/src/makeLottiePlayer.js#L84

Kepro commented 2 years ago

oh right... even actually, do you even need to unsubscribe from events? as you're calling destroy?

mifi commented 2 years ago

I don't know really. Depends on the internal implementation of lottie-web, so it's safer to keep it, I think (in case something changes in the future also)

mifi commented 2 years ago

i'm closing this pr because i'm not sure what is the purpose of it