Create onUnmount in UIPlugin for plugins that require clean up

[Murderlon]

Fixes #3092

• Create and call onUnmount in UIPlugin for plugins that require clean up
• Update webcam stop to clean up when already recording
• Update "Writing plugins" docs
• Move onMount and onUnmount from BasePlugin into UIPlugin
• Update types

[arturi]

I think plugins are only installed once, when they are .used, so we can’t actually uninstall plugin each time user clicks Cancel, because then they can’t return to it?

[Murderlon]

I think plugins are only installed once, when they are .used, so we can’t actually uninstall plugin each time user clicks Cancel, because then they can’t return to it?

Good point. I have now used inversion of control to let plugins define a onClose method which can be used for any clean up that a plugin may require in the dashboard when the user clicks cancel.

Perhaps this should be documented and/or added to UIPlugin?

[arturi]

Yeah, good thinking! Is it similar to onMount, should it be onUnmount then?

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

Line 79 in d613b84

onMount () {}

[Murderlon]

Yeah, good thinking! Is it similar to onMount, should it be onUnmount then?

We already have unmount in UIPlugin. Also good to note that for almost all plugins uninstall and unmount is enough. It's just for our dashboard use case that for instance webcam needs some cleanup when you click "cancel". So I'm not certain yet that it belongs in BasePlugin/UIPlugin. But the pattern of just adding this.onClose to the plugin class is currently only tribal knowledge, if you don't know you won't find it.

[arturi]

Yeah, that’s why I think if we have both this.onClose and this.unmount it might be confusing? The cleanup could be done in this.unmount just as well?

[Murderlon]

Yeah, that’s why I think if we have both this.onClose and this.unmount it might be confusing? The cleanup could be done in this.unmount just as well?

unmount is already defined in UIPlugin and I don't think we want to override this logic?

  unmount () {
    if (this.isTargetDOMEl) {
      this.el?.remove()
    }
  }

For instance, in webcam it is used here:

  uninstall () {
    this.stop()
    this.unmount()
  }

that's why I created onClose but I agree that it is basically the same thing as unmount. I'm also not sure about creating onUnmount in BasePlugin if unmount already exists in UIPlugin. But perhaps it would be a bit better than onClose.

[arturi]

I guess we could just copy whatever is in the unMount when re-defining it, but someone might forget to 🤔

[Murderlon]

I choose to create onUnmount in BasePlugin in the end because:

• It is consistent. onMount already exists there while mount also exists in UIPlugin. Now the same alignment exists for unmount
• It makes sense because mount and unmount do the actual (un)mouting while onX methods are used for additional side effects.

[arturi]

Yeah, this actually does make sense, awesome!

[arturi]

Should we update docs maybe https://uppy.io/docs/writing-plugins/?