@uppy/react: deprecate useUppy

[Murderlon]

Closes #3935
Closes #4220
Closes #4209

Turns out we've been kidding ourselves, a hook for Uppy causes more problems than it solves. At first I thought that most people do not even need Uppy defined inside their component. That's only required if you want dynamic options. But the reality is that there is no use case at all for putting Uppy inside the component because we have setOptions!

The only downside is that implementers have to think about adding this:

useEffect(() => {
  return () => {
    uppy.close({ reason: 'unmount' })
  }
}, [])

I'd argue the same amount of people that used to forget adding useUppy will also forget this, this doesn't chance anything.

This also begs the question again about reason: 'unmount'. This was introduced to let uploads continue in the background, even if the component is unmounted. That should be a decision by the user I'd say, but currently that's baked into the hook. And the API for that is currently not obvious. reason: 'unmount' and suddenly uploads will continue? Am I missing something or why don't we have a more intention revealing API, such as close({ cancelUploads: false })?

[aduh95]

Do we want to highlight import Dashboard from '@uppy/react/lib/Dashboard.js'? We probably want to introduce an exports map to sugar code it, but importing the individual modules makes the most sense otherwise you have to fight with peer dependencies.

[Murderlon]

I was hoping to do a separate PR for the exports map next to this so we don't have to document that workaround

[mifi]

I'm not sure how the current useUppy API even allows for changing Uppy options. As far as I can see, factory() only gets called on the first render, then never again, so if options change, we will not get a new Uppy. Consider this example

function MyComponent () {
  const [tusEndpoint, setTusEndpoint] = useState('https://tusd.tusdemo.net/files' )
  const uppy = useUppy(() => new Uppy().use(Tus, { endpoint: tusEndpoint}))
  return <DashboardModal uppy={uppy} />
}

According to useUppy, if someone calls setTusEndpoint I don't think we will get a new Uppy with the correct endpoint, or am I missing something?

As for removing useUppy because we can just use setOptions instead, I'm not sure you can do everything from setOptions. For example how do you add a plugin?

I still think that the hook could have some value for ease of use, and it allows the user to just think in the "React way" and expect Uppy to work with the options provided. However this assumes that Uppy is properly encapsulated and can handle multiple coexisting instances (e.g. there is no conflicting global state leaking between Uppy instances). For example I know one place where we store global state, and there might be others:

uppy/packages/@uppy/companion-client/src/RequestClient.js

Line 34 in e53d40c

const allowedHeadersCache = new Map()

And the API for that is currently not obvious. reason: 'unmount' and suddenly uploads will continue? Am I missing something or why don't we have a more intention revealing API, such as close({ cancelUploads: false })?

I believe reason: 'unmount' was your own suggestion, because you didn't like that uppy core had knowledge about cancelling uploads: #3575 (comment)

my original implementation was close({ cancelAssemblies: true })

1775410#diff-e522340b6dabc4b8f14b055b742c5c1adca74d15a06f06467956a59b7e106cc2R1248

note that it's not about cancelling uploads, but about cancelling assemblies. uploads will cancel regardless of this option afaik

[Murderlon]

I still think that the hook could have some value for ease of use, and it allows the user to just think in the "React way" and expect Uppy to work with the options provided

I'm not sure how. Here is a codesandbox I made with the two best possible options for the hook to our knowledge. Both don't work in dev mode.
https://codesandbox.io/s/gracious-flower-ztmvrp?file=/src/App.js

I believe reason: 'unmount' was your own suggestion, because you didn't like that uppy core had knowledge about cancelling uploads: #3575 (comment)

Exactly, and I stand by that fact because assemblies are not known to core and are handled in @uppy/transloadit. However, we do have this logic in core. close() calls cancelAll() which has:

uppy/packages/@uppy/core/src/Uppy.js

Lines 809 to 827 in bffd5dc

	cancelAll ({ reason = 'user' } = {}) {
	this.emit('cancel-all', { reason })
	// Only remove existing uploads if user is canceling
	if (reason === 'user') {
	const { files } = this.getState()
	const fileIDs = Object.keys(files)
	if (fileIDs.length) {
	this.removeFiles(fileIDs, 'cancel-all')
	}
	this.setState({
	totalProgress: 0,
	error: null,
	recoveredState: null,
	})
	}
	}

So in fact we do keep all uploads running it seems. Which is not intention revealing.

[aheimlich]

I'm not sure how. Here is a codesandbox I made with the two best possible options for the hook to our knowledge. Both don't work in dev mode.
https://codesandbox.io/s/gracious-flower-ztmvrp?file=/src/App.js

useSafeUppy is, if I understand correctly, what the React team recommends according to reactwg/react-18#18. The problem is that, in order to use it correctly, all of the Uppy React components would need to be modified to account for the fact that you're passing a factory function to them, and not an Uppy instance.

I have a code sandbox where I'm playing around with some things.

https://codesandbox.io/s/optimistic-water-twsgft?file=/src/App.js

It mostly works, but I can't seem to get Uppy to play nicely with HMR (I think?).

This is of course, a major BC break for the React components.

[mifi]

So in fact we do keep all uploads running it seems. Which is not intention revealing.

Oh. I think this is because if the user is uploading via Google Drive or similar, we don't want the uploads to cancel just because the user navigates away (we don't want the users to sit around and wait for something they really don't have to wait for).

The ideal useUppy hook API I would envision is a fully declarative API where the hook "just provides" a ready uppy instance for you with the wanted options and plugins, so you don't have to deal with all this logic manually. You can just feed it options from your react state or config or whatever. It will then create an uppy with the desired plugins, or call setOptions, removePlugin, uppy.use, getPlugin.setOptions etc on the existing uppy instance if the changes don't require uppy to be reloaded. IMO this is the true react way.

const [maxNumberOfFiles, setMaxNumberOfFiles] = useState()
const [tusEndpoint, setTusEndpoint] = useState()

const uppy = useUppy({
    options: { restrictions: { maxNumberOfFiles } },
    plugins: [
      { plugin: Tus, options: { endpoint: tusEndpoint } },
    ]
  })

return <Dashboard uppy={uppy} />

I experimented a bit with such an implementation. obviously it's really far from done but it shows the concept. You can copy this into the codesandbox:

import { useRef, useEffect, useState, useMemo, useCallback } from "react";
import { Dashboard } from "@uppy/react";
import Tus from '@uppy/tus'

import Uppy from "@uppy/core";

import "@uppy/dashboard/dist/style.css";
import "@uppy/core/dist/style.css";

function useUppy(props) {
  const lastProps = useRef()
  const uppyRef = useRef()

  const makeUppy = useCallback(() => {
    const { options, plugins } = props

    if (!lastProps.current) {
      console.log('creating uppy')
      uppyRef.current = new Uppy(options)
    } else {
      const { restrictions } = options

      const lastOptions = lastProps.current.options
      if (lastOptions.restrictions !== restrictions) {
        console.log('updating uppy options')
        uppyRef.current.setOptions({ restrictions })
      }
    }

    const lastPlugins = lastProps.current ? lastProps.current.plugins : []
    lastPlugins.forEach(({ plugin }) => {
      if (!plugins.find((p) => p.plugin === plugin)) {
        console.log('removing plugin', plugin.prototype.constructor.name)
        const instance = uppyRef.current.getPlugin(plugin.prototype.constructor.name)
        uppyRef.current.removePlugin(instance)
      }
    })

    plugins.forEach(({ plugin, options }) => {
      const lastPlugin = lastPlugins.find((p) => p.plugin === plugin)
      if (!lastPlugin) {
        console.log('adding plugin', plugin.prototype.constructor.name)
        uppyRef.current.use(plugin, options)
      } else {
        if (lastPlugin.options.endpoint !== options.endpoint) {
          console.log('updating plugin options', plugin.prototype.constructor.name, options)
          const instance = uppyRef.current.getPlugin(plugin.prototype.constructor.name)
          console.log(instance)
          instance.setOptions(options)
        }
      }
    })

    lastProps.current = props

    return uppyRef.current
  }, [props])

  const [uppy, setUppy] = useState();

  useEffect(() => {
    setUppy(makeUppy())
  }, [makeUppy])

  useEffect(() => () => {
    if (uppy != null) uppy.close()
  }, [uppy])

  return uppy;
}

function Component({ uppy }) {
  return (
    <div className="App">
      <h1>Hello CodeSandbox</h1>
      <Dashboard uppy={uppy} />
    </div>
  );
}

// const uppy = new Uppy({ id: "dashboard" })
export default function App(props) {
  const [tusEndpoint, setTusEndpoint] = useState('https://tusd.tusdemo.net/files');
  const [maxNumberOfFiles, setMaxNumberOfFiles] = useState(1);

  const uppy = useUppy({
    options: { restrictions: { maxNumberOfFiles } },
    plugins: [
      { plugin: Tus, options: { endpoint: tusEndpoint } },
    ]
  });

  if (!uppy) return null // todo

  return (
    <>
      <div id="drag-drop-area" />

      <select onChange={(e) => setMaxNumberOfFiles(parseInt(e.target.value, 10))} value={maxNumberOfFiles}>
        {[1,2,3,4].map((v) => <option key={v} value={v}>{v}</option>)}
      </select>

      <select onChange={(e) => setTusEndpoint(e.target.value)} value={tusEndpoint}>
        <option value="https://tusd.tusdemo.net/files">https://tusd.tusdemo.net/files</option>
        <option value="https://tusd2.tusdemo.net/files">https://tusd2.tusdemo.net/files</option>
      </select>

      <Component uppy={uppy} />
    </>
  );
}

[Murderlon]

The ideal useUppy hook API I would envision is a fully declarative API where the hook "just provides" a ready uppy instance for you with the wanted options and plugins, so you don't have to deal with all this logic manually. You can just feed it options from your react state or config or whatever. It will then create an uppy with the desired plugins, or call setOptions, removePlugin, uppy.use, getPlugin.setOptions etc on the existing uppy instance if the changes don't require uppy to be reloaded. IMO this is the true react way.

For now, this seems a bit over-engineered and deviates from the way we use plugins. I think the way you use Uppy shouldn't change too much. If we continue with a hook, I'd say we keep it simple for the 90% use case and think about it again if people start complaining. So far, no one has (except for that it's broken of course).

---

@aheimlich thank you for looking into this. I'll give your solution a closer look, but it looks promising on a first glance.

[nickrttn]

useSafeUppy is, if I understand correctly, what the React team recommends according to reactwg/react-18#18.

The caveat here is that the factory function that useSafeUppy returns should only be used in event callbacks or effects, as noted in the comments of the reference implementation by @bvaughn. It also explicitly states it shouldn't be called during render.

Relying on that ref to change when you need it to during rendering most likely means you are relying on a side-effect somewhere.

How about something like

function useUppy(onCreateOrChange: (uppyInstance: Uppy) => Uppy, deps: any[]) {
  const [uppy, setUppy] = useState(() => {
    const uppy = new Uppy()
    return onCreateOrChange(uppy)
  });

  useEffect(() => {
    setUppy(onCreateOrChange(uppy))
  }, deps);

  return uppy;
}

This can probably be improved a lot API-wise, but to me this follows React paradigms. This could be used almost like a useEffect hook with a return value. Refs are really only meant as an escape hatch if you can't use state.

[bvaughn]

Not really familiar with this code but what @nickrttn said sounds correct to me– useUppy seems flawed by default in its current form because it's reading from a ref during render. Refs aren't managed by React like state is, which means they may get out of sync if you have multiple updates scheduled (which could cause "tearing").

Moving to state+effect would avoid that issue.

[mifi]

Alternatively this version, which would allow eslint rule of hooks (react-hooks/exhaustive-deps) to help developers:

function useUppy(onCreateOrChange: (uppyInstance: Uppy) => Uppy) {
  const [uppy, setUppy] = useState(() => {
    const uppy = new Uppy()
    return onCreateOrChange(uppy)
  });

  useEffect(() => {
    setUppy(onCreateOrChange(uppy))
  }, [onCreateOrChange]);

  return uppy;
}

..then use the hook like this and eslint would warn about using deps not in the useCallback deps list:

const [restrictions, setRestrictions] = useState();

const onChangeOrCreate = useCallback((uppy) => {
  return uppy.setOptions({ restrictions })
}, []) // eslint error: react hook has a missing dependency restrictions

uppy = useUppy(onChangeOrCreate)

[Murderlon]

In my opinion, we either make the hook sufficient that you don't need to add more state and memoization or we just write docs how to do it yourself. Otherwise, what's the gain of the hook? The answer to whether we like developers to control whether to continue or cancel uploads on unmount would also play a large part in that decision I'd say, as that's harder to do if you hide it inside an useUppy hook.

[mifi]

Here's an example of how to use @nickrttn's hook to enable/disable plugins and change restrictions on the fly:

  const [maxNumberOfFiles, setMaxNumberOfFiles] = useState();
  const [showWebcam, setShowWebcam] = useState(true);

  const uppy = useUppy((uppy) => {
    if (showWebcam && !uppy.getPlugin('Webcam')) {
      uppy.use(Webcam)
    }
    if (!showWebcam && uppy.getPlugin('Webcam')) {
      uppy.removePlugin(uppy.getPlugin('Webcam'))
    }
    uppy.setOptions({ restrictions: { maxNumberOfFiles } })

    return uppy
  }, [maxNumberOfFiles, showWebcam])

  return (
    <div>
      <button onClick={() => setShowWebcam((v) => !v)}>{showWebcam ? 'webcam on' : 'webcam off'}</button>

      <select onChange={(e) => setMaxNumberOfFiles(parseInt(e.target.value, 10))}>
        {Array(10).fill(0).map((v, i) => <option key={i} value={i}>{i}</option>)}
      </select>

      <Dashboard uppy={uppy} plugins={showWebcam ? ["Webcam"] : []} />
    </div>
  );

it could be made much more compact and easier to use if Uppy's API was idempotent (if calling uppy.use(plugin) when the plugin is already added would just return silently instead of erroring, same with removePlugin)

One potential issue with Nick's hook is that onCreateOrChange gets called twice:

• First on the first render, when useState calls its initialiser
• Then once more in the useEffect on mount

We could optimize this, but because Uppy's react components expect uppy to be defined at render time, we would have to provide it during render. (or check for if (uppy == null) return null; inside render but that's not very elegant and causes an empty initial render)

[Murderlon]

A couple things need to change for the hook to be acceptable in my opinion. This increases the scope outside of the hook.

• new Uppy() should not be initialized inside the hook (which happens in the latest proposal). This makes passing options to core a very odd thing to do. You would have to do a uppy.setOptions() inside the useUppy callback and that's straying too far into a different pattern. How to use Uppy should be familiar across the stacks, this is not intention revealing and inconsistent.
• The user should not have to do the whole this.getPlugin() dance to conditionally set a plugin. It's verbose, unnecessary, and error-prone. We should change core to not crash when you reset a plugin and instead ignore it. This wouldn't be a breaking change.
• The user should not have to have to think about adding a plugins prop with plugin IDs which is a odd thing to do inside React to let a component know it should magically render some plugin inside it. It should be done with a ref instead. This requires a bit more thinking, as ref is only defined after render and the uppy instance is expected sooner.
• We should let the implementer control whether they want to continue or cancel uploads. I'm thinking we should add cancelUploads: boolean as an option, which would overlap with reason: "unmount" until the next major.

Put all together, using useUppy would look like this:

// Simple use case
const dashboardRef = useRef();
const uppy = useUppy(() => new Uppy().use(Webcam, { target: dashboardRef }), []);

useEffect(() => {
  return () => {
    // you are in control whether you want to cancel uploads or not.
    uppy.close({ cancelUploads: false });
  };
}, []);

const dashboardRef = useRef();
const [maxNumberOfFiles, setMaxNumberOfFiles] = useState();
const [showWebcam, setShowWebcam] = useState(true);

// Do things conditionally
const uppy = useUppy(() => {
  const uppy = new Uppy();

  if (showWebcam) uppy.use(Webcam, { target: dashboardRef });
  else uppy.removePlugin(uppy.getPlugin("Webcam"));

  uppy.setOptions({ restrictions: { maxNumberOfFiles } });

  return uppy;
}, [maxNumberOfFiles, showWebcam]);

// useEffect to cleanup...

[mifi]

I think the problem with your approach of the user providing new Uppy themselves is that every time deps changes, a new Uppy would be recreated, unless the user adds an if-check, but that’s not very elegant. Other than that I agree with most points, but I think closing uppy on unmount could be incorporated into the hook (with an option)

[Murderlon]

I think the problem with your approach of the user providing new Uppy themselves is that every time deps changes, a new Uppy would be recreated

But that's what we want and need? Because of the clean up function of the effect, we need a new instance because the old is now closed. If you want to keep Uppy from closing, you would do it as you would with any other React component: prevent it from re-rendering.

[mifi]

Whenever a new Uppy is created, the UI state will reset. Like which files you selected etc. I think that’s not ideal. I think ideally when any deps of the useUppy hook change, the same uppy instance should remain, if possible, and then the user can setOptions to change whatever they want. Only when the component unmounts shall close be called

[nickrttn]

Tricky thing here is that useEffect does not exactly have mount/unmount semantics. It runs on mount and on unmount, but it can also be caused to run by other reasons like fast refresh and strict mode. I think the best course of action is to:

• advise users to create an instance of Uppy for each use case outside of the React render loop
• if they need to dynamically change Uppy options, to try to do so via event handlers as much as possible and only if there is no other option, to do so via an effect.
• I think this would most closely follow the line of reasoning in https://beta.reactjs.org/learn/you-might-not-need-an-effect

Looking at the examples, the potential solutions we are coming up with are pretty standard forms of React hooks and do not manage any complexities inherent to Uppy, ie. they are not really necessary imo. The only thing about them is the close on unmount, but as @mifi points out, that might not be desirable

[Murderlon]

But uppy.close() must be called at some point. It cleans up event listeners and other things that would leak. And if you call it once, then the uppy instance becomes unusable. You can't define it outside of the component because it would never clean by itself, and if you clean it inside the render loop then after a single re-render the instance is unusable from that point on. It can't be revived. Hence I'm still thinking we need a hook and that's why it doesn't matter that it gets recreated.

[mifi]

I think if we add this inside nick’s hook:

useEffect(() => () => uppy.close({ reason }), [uppy]);

Then it will clean up whenever the hook unmounts. This would allow the hook to create new Uppy only once in the beginning, and then cleanup on unmount.

[nickrttn]

But uppy.close() must be called at some point. It cleans up event listeners and other things that would leak. And if you call it once, then the uppy instance becomes unusable. You can't define it outside of the component because it would never clean by itself, and if you clean it inside the render loop then after a single re-render the instance is unusable from that point on. It can't be revived. Hence I'm still thinking we need a hook and that's why it doesn't matter that it gets recreated.

I'm not sure I understand. Why would it leak if you create a single instance in the global scope? Leaks in React apps are commonly introduced by side effects that are not cleaned up. In this case, you only have a single instance (per use case) and you wouldn't need to clean it up except maybe on an unload event.

It's like instantiating a worker outside of a React component, or a client for error monitoring. Both are commonly done, simplify the structure of your code and reduce the surface area for bugs.

[Murderlon]

Alright, I thought it made sense to clean up event listeners as you probably only use Uppy on one page and then they live on. But we can ignore it, it's probably harmless.

[Murderlon]

Docs now ready for final review.

[nickrttn]

Alright, I thought it made sense to clean up event listeners as you probably only use Uppy on one page and then they live on. But we can ignore it, it's probably harmless.

Could you clean up the listeners that are attached to the rendered unmounting DOM elements without closing the Uppy instance?

[Murderlon]

Could you clean up the listeners that are attached to the rendered unmounting DOM elements without closing the Uppy instance?

They live in Uppy core, not the UI elements (although they could have their own). Don't think we can do that without having an effect that would enable them again. Looking into the code it's actually less than I thought, we have a massive addListeners function but most subscribe to the namespace-emitter package and only two to window.

[ahardin]

Am I incorrect in believing that some of the discussion above should lead to a solution for #4230 as Murderlon seemed to indicate?

This PR is exclusively for deprecating useUppy(), but most of the ideas discussed above involve using useUppy(). I think we need more guidance on the right way to get Dashboard to respond to props changes.

Also, many/all of the React docs pages still link to https://uppy.io/docs/react/initializing, which was removed in this PR. I would be happy to submit a PR to fix that, but I'm fairly puzzled at this point.

[mifi]

Are you seeing the style issues still? Even without using the hook? I think you can initialize uppy ouside the react component and then have a useEffect that reacts to state that will call setOptions on the uppy instance. Does that help? Yea we need to fix the broken docs. I think we wanted to possibly design a new, better hook, but that was out of the scope of this PR, so we just wanted to deprecate this broken version first, so people stop using it.

[ahardin]

Thanks @mifi. Yes - that approach appears to work. Thank you!

The only thing that looks wrong is the "browseFiles" link in the drag/drop area of the dashboard remains fully clickable after using setOptions({disabled: true}). The disabled style gets applied, but it's not actually disabled.

You can see this on the example page of the docs: https://uppy.io/examples/dashboard/ Just check the "disabled" checkbox and then click the grayed out "browse files" link.

This is probably outside the scope of this PR (also doesn't seem limited to React). Shall I open an issue for this?

[mifi]

Yes please open am issue for that, thanks!

[arturi]

@ahardin thanks for reporting, fixing in #4292