-
Notifications
You must be signed in to change notification settings - Fork 29
fix: Improve robustness of hooks and the Canvas component #227
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
base: main
Are you sure you want to change the base?
Conversation
…soon as the size is available.
Thank you for consolidating this. Could provide me with some scenarios that are currently broken that I could use for testing? |
It would be great to have some testing examples since these changes are a bit complex, and those examples will help in understanding some of the decisions made 🙌 |
@iwoplaza to provide a bit of context, useCanvasEffect was created because on paper the size of the view wouldn't be know fast enough. On Fabric, we don't need this hook really (I think, need to double check). Should we remove this hook and just document it as a paper only hook? We need to decouple from two things:
Let me know your thoughts. |
Changes
useCanvasEffect
API and behavioral changesUsers of
useCanvasEffect
are responsible for the stability of the callback function. This ensures that, in the default case (not wrapping the input in auseCallback
), we are not depending on stale values.The callback is provided with a few values, like the canvas and context. An abort signal is also passed in, allowing the callback to return early if the component was already unmounted while asynchronous work was being done.
Allowing more than one
.whenReady
callback to be registeredAfter the changes in this PR, calling
.whenReady
multiple times will register each callback in a list, instead of remembering only the latest one.Making hooks resilient to race conditions
In case a hook uses
useEffect
with asynchronous behavior, there are cases where it can re-ran before the function completes. This can introduce bugs that are hard to reproduce, because they depend on which hook call finishes first. By only committing work from the hook that has not been unmounted by the time we finish all async work, we can avoid this.There were also a few cases where information was stored in a
useRef
, but it's use was actually scoped to a single useEffect invocation (not spanning useEffect invocation), so it was moved to just be a local variable of the useEffect callback.Using
Immediate
values to act fasterSince we're using the latest
size
in methods of the CanvasRef imperative handle, we can get their latest value without recomputing the value of the imperative handle reactively.