@uppy/react: introduce useUppyEvent

[Murderlon]

We already have useUppyState (#5059) but there is a need for useUppyEvent. Considering the following code, which is very common when building with React and Uppy.

const [transloaditResult, setTransloaditResult] = useState<ManuallyTypeThis>([])
const [stepName, result, assembly] = transloaditResult

useEffect(() => {
  function handler(stepName, result, assembly) {
    setTransloaditResult([stepName, result, assembly)
  }
  uppy.on("transloadit:result", handler);

  return () => {
    uppy.off("transloadit:result", handler);
  };
}, [uppy, setTransloaditResult]);

With this PR it's as simple as

const [results, clearResults] = useUppyEvent(uppy, 'transloadit:result')
const [stepName, result, assembly] = results

// optional: listen to another event to clear the results state
useUppyEvent(uppy, 'cancel-all', clearResults)

Note

One thing thing to consider is that values are stale until the next event (if that ever comes). Depending on your use case that may be expected or not. For instance, you listen for transloadit:result but once the user clicks "Done" in dashboard and the state is reset, you still have the stale reference to results of that event. That's why we return a "reset" function to clear the state.

Another thing to know is that not all events return something. In that case it's very likely because you want to do something else when that event triggers, not have a value in state. This is possible with a third argument which is a callback.

[github-actions]

Diff output files

diff --git a/packages/@uppy/react/lib/index.js b/packages/@uppy/react/lib/index.js
index 2739da8..951a3db 100644
--- a/packages/@uppy/react/lib/index.js
+++ b/packages/@uppy/react/lib/index.js
@@ -4,4 +4,5 @@ export { default as DragDrop } from "./DragDrop.js";
 export { default as FileInput } from "./FileInput.js";
 export { default as ProgressBar } from "./ProgressBar.js";
 export { default as StatusBar } from "./StatusBar.js";
+export { default as useUppyEvent } from "./useUppyEvent.js";
 export { default as useUppyState } from "./useUppyState.js";

[mifi]

Not sure about mixing state (like assemblyResults) into an event hook. State seems to me like something that should instead be exposed in useUppyState. But as a way to hook into events, it's a very useful hook, like

useUppyEvent(uppy, 'transloadit:result', (r) => console.log('done', r)),

[Murderlon]

I agree and I would not have done that if Uppy holds everything in state, but in the case of events that's not always the case, yet you do need those event results to be stateful when building a UI. The reason why only doing your proposal is not ideal is because you have to manually type everything again:

const [transloaditResult, setTransloaditResult] = useState<UppyEventMap<M,B>['transloadit:result'] | []>([])

useUppyEvent(uppy, 'transloadit:result', setTransloaditResult),

Both my current design and this one don't feel quite right. But at least the current design takes away most complexity.

[mifi]

maybe we should instead introduce some new state then, where it makes sense? something like lastTransloaditResult.

I think having state stored inside the event hook could lead to unnecessary performance degradation. for example if someone useUppyEvent('progress', (p) => doSomethingWithProgress(p)), if this progress event gets fired multiple times per second (depending on uploader), it would cause a re-render on every single event trigger (because of the setState call inside the hook), and if they have a large/complex component this could cause significant slowdown. And a re-render would be kind of unexpected when you're just subscribing to an event listener

[Murderlon]

If you want to subscribe to progress, you want to re-render too, that's kind of the point. If you mean to showcase only using the callback instead of the returned state, then it's most likely because you use some stateful callback from a parent component, which means the parent and child are re-rendered. Again, that would be intended as you do something stateful in the parent UI.

maybe we should instead introduce some new state then, where it makes sense?

Uppy core has no knowledge of transloadit specific state so the types in core can't know this. We could try to populate the state modules and interface merging, but that's a very different approach and it simply moves the problem: now you move the stale state from useUppyEvent inside Uppy. We can't guess when to invalidate all this new state which used to be fire-and-forget events.

[mifi]

If you want to subscribe to progress, you want to re-render too, that's kind of the point.

Often when subcribing to events that happen very frequently, one wants to throttle/debounce these events before updating the UI (updating state), because else there would be too many re-renders, causing slowdowns. I can think of events like progress as well as file-added, upload-progress but there may be others too. I remember these problems have occurred in Uppy in the past where events were emitted very fast causing problems. but maybe we just tell users to instead manually uppy.on('event') if they hit these problems then

[Murderlon]

All progress events are already throttled by us. Events like file-added should never be throttled imo.

[mifi]

to illustrate the problem: if someone wants to useUppyEvent('file-added', (file) => myListOfFiles.push(file)), and the user adds 1000 files, then that will cause setState to be called 1000 times, which might be unexpected for developer who uses this hook (and might cause a lock-up in the react ui)

[Murderlon]

That example doesn't make sense because you should never do .push on something from useState but call the state setter and I highly doubt people want global variables of myListOfFiles.

I think you're thinking about it the wrong way, when you access state (one way or another) it's because you need it because your UI is stateful. There is no use case where it isn't stateful, then you simply don't use it at all.

also for your example you probably want useUppyState(uppy, (state) => state.files) instead of file-added.

[mifi]

yes, maybe the file-added event is not very useful in the context of react

[Murderlon]

So is this good to go or are there concrete requested changes?

[mifi]

Lgtm