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

Add onUpdate callback #9

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

michellocana
Copy link
Contributor

  • onUpdate prop added on AutoTextSize React component
  • onUpdate option added on autoTextSize vanilla method
  • yalc added on devDependencies
  • Fix errors while trying to run the example project
Screencast.from.30-11-2023.17.22.12.webm

@ViktorQvarfordt
Copy link
Member

Thank you! Just one question: Is there a risk that adding onUpdate to the dependency array will re-trigger the useEffect very often since we don't pass stable references for the onUpdate function? I.e., useCallback might be needed.

@michellocana
Copy link
Contributor Author

Thank you! Just one question: Is there a risk that adding onUpdate to the dependency array will re-trigger the useEffect very often since we don't pass stable references for the onUpdate function? I.e., useCallback might be needed.

Good catch, we should probably wrap this callback in a ref, so we always get the latest value without impacting useEffect calls

@michellocana
Copy link
Contributor Author

@ViktorQvarfordt done!


useEffect(() => updateTextSizeRef.current?.(), [children]);

useImperativeHandle(onUpdateRef, () => onUpdate)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This usage feels a bit magic. Perhaps we should move the responsibility of memorizing the onUpdate function to the user of the hook instead? I mean they'd pass useCallback(f, []) instead of f

Copy link
Contributor Author

@michellocana michellocana Dec 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm okay with letting the user wrap the callback on an useCallback, i just used a ref to keep the callback updated because a lot of people don't have the habit to use it, but maybe we can just add a warning in the README and pass this responsibility to the user

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, perhaps your approach would be good. I'm not familiar with the use of useImperativeHandle in this way. I would have expected:

const onUpdateRef = useRef()

useEffect(() => {
  onUpdateRef.current = onUpdateRef
}, [onUpdateRef])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, both the useEffect and useImperativeHandle approaches do the same in this case, but I can change to useEffect if you want, no problem

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @ViktorQvarfordt, sorry for the huge delay, I've just updated the branch with the origin one, let me know if you still want me to change the useImperativeHandle call.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, most people aren't familiar with this hook. Simple is good!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

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.

2 participants