Abstract uploading logic into Uploader class

[Murderlon]

Second PR in the series to improve core.

Future improvements

• Do the same for events

[Murderlon]

This one was a bit harder than #3532. With the Restricter class, there was a clearer separation between logic and side-effects. This gets a bit more fuzzy with uploads because almost everything is side-effects; uploads continue or happen through getting and setting state. For this reason left out all the pause/retry/cancel methods because they all do set state and emit events (something that feels more like it belongs in core) and because those methods need to remain public.

I think it's in a state of review but feel free to share thoughts on to further tackle upload separation.

[Murderlon]

I agree with your feedback on state. This is something I struggled with, how to separate state management. I'll try to think about that again and then we can reevaluate.

[Murderlon]

Update: I strictly separated the logic from the side-effects and only getOpts is passed to the class, which I think is reasonable and makes sense. However, as running/cancelling/retrying upload is nothing more than getting/setting state, which is perhaps normally considered as side effects, I now treat as the logic itself. Anything else like logging, emitting, or other non-upload state updates stay in core.

I get and set state through opts.store and to me it seems reasonable to pass around a centralized store to delegated classes.

Lastly, going deeper in core made it clear to me that almost everything we do is in fact just getting and setting state. Actual logic resides in plugins. Therefor I could also be interesting to think more in state management patterns for a future refactor because all methods do is constantly repeat the same tricks with getting and setting state.

[Murderlon]

I understand your concerns. First, consider my last comment:

Lastly, going deeper in core made it clear to me that almost everything we do is in fact just getting and setting state. Actual logic resides in plugins. Therefor I could also be interesting to think more in state management patterns for a future refactor because all methods do is constantly repeat the same tricks with getting and setting state.

So we know that uppy core is almost nothing besides getting and setting state. Triggering things, like upload, retry, etc is really just setting state for the most part.

Now there are two things we can do:

1. Group logic that changes together. This is an attempt at that in which I treat the external store (which I conceptually see as I centralized store, and thus not only owned by Uppy.js) as something that can also be used in delegated classes. So here we have everything regarding uploading state in a separate file, and changes to uploading logic could be mostly safely assumed that you just change this file, as uppy core will only deal with the side effects of it (like emitting or logging). This is not inherently a bad idea, in React you set and update state from all kinds of places, top level or down in a component. Same goes for functional programming.
2. Keep everything in Uppy core, but create lots of state utilities. As everything is getting and setting state, and if you look closer you'll see we constantly do the same (duplicated) tricks with state, another way to go about this focusing on state management. So that would not mean mutating functions like retryAll and using that in uppy.retryAll, it would mean retryAll in uppy would be a composition of state patterns. This could be something like lenses, or whatever works for Uppy.

Regarding the current state of Uploader, I don't think it necessarily bad. But while it definitely made sense for Restricter, and a bit for Uploader, the next refactor isn't as obvious if you hold onto the same idea. While the second approach leaves things more flexible, it does keep growing Uppy core.

What do you think?

[mifi]

I think if we are to make our own central state management solution, we should try to make an immutable state API, like setState((existingState) => ({...existingState, newProp: true })), maybe like React has. Then different parts of uppy can subscribe to updates to the state tree, similar to useEffect is react. But that may also be over-engineering for Uppy's use case.

If we want to make state update utility functions, I definitely think they should be pure (e.g. operate on immutable objects and not modify anything in arguments sent to it).

I don't really see Uppy.js as too big or unwieldy in its current state. I think it's often inevitable that a central coordinator class/module (e.g. the public API) will have to contain a relatively large amount of code and methods in order to coordinate everything. I think code should be rewritten to async/await, and some complex pure state transformation logic could possibly be pulled out. So I think I would prefer option 2. Maybe we should weigh in some other people's opinions too.