Fix uppy.close()-related issues that occurred in Dashboard

[arturi]

Fixes #396

Check that plugin acquired with getPlugin() exists before calling render() on it.

[goto-bus-stop]

i wonder what the root cause is here. is nanoraf calling the updateUI function after we unmounted?

[arturi]

i wonder what the root cause is here. is nanoraf calling the updateUI function after we unmounted?

Yes, I think that is it, and it also the reason for what I mentioned in Slack:

I’ve been working on improving accessibility for the Uppy Dashboard, and one of the things this requires is managing focus. I’ve found that if I add a “find an element and focus on it” code to openModal, it does nothing — won’t focus. After multiple tests and pulled hairs I’ve found that it works perfectly when I remove nanoraf from the update method :thinking_face:

[arturi]

Makes me wonder if all of our troubles will be solved by just going with Preact. I am 70% sold at this point anyway, for reasons I can list again:

• better syntax highlighting (if we also go JSX, which is probably a good idea; allows us to use things like https://www.npmjs.com/package/eslint-plugin-jsx-a11y too)
• easier animations
• better lifecycle events
• passing refs to elements
• extra network requests with base64 urls issue
• nanoraf issues (could be solved by PRing nanoraf)
• active community (yo-yo is not actively updated, only choo is, but Preact has hit 95% of edge-cases that we are still discovering ourselves by sticking with bel/yo-yo).

[goto-bus-stop]

I imagine there are other places where this might be biting us then. I think we need to cancel the animation frame on unmount somehow. We could PR nanoraf to add a .cancel() method or add a condition in our updateUI function to check if we're still mounted.

How do you feel about the suggestion in the latter half in this comment btw? #297 (comment) I think that'd be a nice way to have a migration path for us and it would allow others to use whatever view library they prefer for their own plugins.

[arturi]

condition in our updateUI function to check if we're still mounted.

will look into that as a quick solution, thanks!

How do you feel about the suggestion in the latter half in this comment btw? #297 (comment) I think that'd be a nice way to have a migration path for us and it would allow others to use whatever view library they prefer for their own plugins.

Replied there, agreed! I like this approach a lot.

[arturi]

So how shall we handle this? Should I merge this PR or just disable nanoraf for a while? And then we maybe move to Preact?

[goto-bus-stop]

Disabling nanoraf would be really bad for performance, especially when adding new files. I think a condition inside updateUI would be quite simple and effective. Maybe by checking a this._installed property that is set inside the install and uninstall methods

[goto-bus-stop]

ow, i see #414 would also remove nanoraf. i don't have a better idea for that use case short term, so maybe we'll have to take the perf hit for now. we can close this pr then, i think, and get the same fix from #414.