transloadit / uppy

The next open source file uploader for web browsers :dog:
https://uppy.io
MIT License
29.2k stars 2.01k forks source link

Simplify Uppy #5509

Closed mifi closed 5 days ago

mifi commented 6 days ago

Problem

Uppy has become somewhat unwieldy and has much more packages than it needs to have, being a relatively simple thing, an uploader library.

Solution

IMO we should reduce the number of packages/plugins.

Candidates:

Maybe more controversial:

Somewhat related to #5379

Alternatives

Only some of the above suggestions.

Murderlon commented 6 days ago

I agree with the problems but not with the outcome. For a long time I've been weighing the cost/benefit of a complete rewrite vs. incremental improvement. Complete rewrites almost never make sense for mature, big SaaS platforms but for OSS libraries it's a bit different and more common. Nonetheless I dismissed that idea after the refactor to TS, which took longer than expected, and to really approach the justification from a user perspective.

Most of your problems are related to how we experience the codebase. While that should be taken seriously as well, that only comes second in importance to what value we deliver to users. The majority of your suggestions is just moving around code in a time consuming way.

@uppy/store-default: merge into core

No strong feelings about this. It's an internal-only dependency so users don't care.

@uppy/locales merge into core, but still allow people to import specific locales (paths)

Hard disagree. 90% of the users don't need another language and we inflate the bundle size significantly.

@uppy/utils: merge into core

Note that versioning and releasing should be taken into account. Plugins can depend on different versions of utils but core must be the same. That means tooling needs to be build to release all plugins to bump the core dependency whenever core changes. So not sure about this.

@uppy/golden-retriever, @uppy/compressor, @uppy/thumbnail-generator: I feel like these should be part of core functionality in Uppy. as it is now, it has broken so many times because of the complexity and because we forget to test with it.

Has it broken that many times? Sounds like an XY problem, if something breaks moving the code together doesn't make it not break. It's like you said, forget to test with it, so we should write tests and have that certainty with less effort?

@uppy/remote-sources: remove this plugin - instead make the API for adding multiple remote sources to uppy core easier to use

Open to the idea. But remember that very few users need all or almost all remote sources so having 2 maybe 3 times .use() some plugin is totally fine in my opinion. Have you seen the average amount of .use() in the unified ecosystem, including our own linter? No one complained, it's not our biggest problem.

@uppy/provider-views, @uppy/companion-client: merge into one (already suggested in #4936)

I agree with this

@uppy/file-input @uppy/drag-drop, and @uppy/drop-target - similarly to @uppy/form, doesn't have to be a plugin. Remove the plugin and make an easy to use API with good example code, or make a headless component @uppy/status-bar, @uppy/progress-bar: merge into dashboard, instead allow people to make their own UI by providing headless components or easy to integrate state with example code

This is actually the biggest user pain point of them all. That's why I created #5379, which deserves its own issue and discussion. That's a massive undertaking which requires lots of thought and breakdown, more than a single sentence here.

@uppy/form gives people too little freedom. should either be removed and converted into example code, or made into a headless component

This is a great and essential plugin. Also it's not a component? It's about managing your existing form submit with file uploads.

Superseded by #5379

Superseded by #5379

We can consider this yes šŸ‘

It's the CDN bundle, another essential package.


I've been thinking about Uppy's vision for a long, long time. And what I settled on as the most value for invested time is a future where users:

  1. ...have a fully typed experience āœ…
  2. ...can compose their UI according to their custom needs and branding
    • 5379

  3. ...can integrate seamlessly on the frontend and backend while validating restrictions on both sides
    • 5033

  4. ...find exactly what they want through succinct, well written, interactive documentation.

That's it. Laser focus on delivering value, not moving code around for us, no assumptions on what could be better but based on actual concerns across GitHub, Community Forum, and Intercom.

mifi commented 6 days ago

I agree with the problems but not with the outcome. For a long time I've been weighing the cost/benefit of a complete rewrite vs. incremental improvement. Complete rewrites almost never make sense for mature, big SaaS platforms but for OSS libraries it's a bit different and more common. Nonetheless I dismissed that idea after the refactor to TS, which took longer than expected, and to really approach the justification from a user perspective.

I'm not talking about a complete rewrite though, but yea I see that some could be quite big changes. However some are not that crazy and I think can still be done incrementally. but they are all very breaking changes for the user of uppy (developer, not end user). I think with proper types, we can more confidently do refactors like this, so it was a good investment even though it took a long time.

only comes second in importance to what value we deliver to users

not sure what you mean by users, but if you mean end users who use the webapp that in turn uses Uppy, yea they don't care much about the code. when i say user i'm thinking about the developer who is using Uppy in their project. I think having a simple structure of plugins/packages to choose from makes it easier for them to use Uppy. When I see 50 packages a get a bit overwhelmed. but yea i agree it's mostly of a problem for us (and contributors).

@uppy/locales merge into core, but still allow people to import specific locales (paths)

Hard disagree. 90% of the users don't need another language and we inflate the bundle size significantly.

Even if the total bundle size of the npm package @uppy/core increases, I don't think that's a big deal, as long as we export each locale as a separate path so that developers can import locale files individually. I think virtually all web devs use a bundler anyways, and bundlers will only include the code that has been imported. (except for those who use CDN, but in the cdn bundle we already include all languages right?)

Note that versioning and releasing should be taken into account. Plugins can depend on different versions of utils but core must be the same. That means tooling needs to be build to release all plugins to bump the core dependency whenever core changes. So not sure about this.

I think npm/semver solves this?

Open to the idea. But remember that very few users need all or almost all remote sources so having 2 maybe 3 times .use() some plugin is totally fine in my opinion. Have you seen the average amount of .use() in the unified ecosystem, including our own linter? No one complained, it's not our biggest problem.

Yes, so I think we agree then, right? calling uppy.use on each provider they want should be totally fine and the remote-sources plugin is not really necessary.

It's the CDN bundle, another essential package.

not sure why the CDN bundle needs to be an npm module that we publish to npm. can the CDN bundle be installed from npm?

I agree with your priorities, but I still think some of the things here could be done without too much effort (although still probably breaking changes). for me what triggered this need was:

Murderlon commented 5 days ago

I mostly agree with you but as mentioned I think we have bigger fish to fry. I appreciate you taking the time in sharing thoughts and suggestions but in the context of having barely any maintainer hours across the board, it would help us a lot more if you wrote an issue per problem/solution instead of a massive one that can practically never be considered done.

For your own understanding and ours, I highly recommend using the pitch format (without the sketches) to write issue(s) like these. Another example is how we present bigger initiatives within unified with RFCs. Or the effort I put into #5379.

If you'd think critically and you'd only get to make one, at most two, initiatives to improve some of the problems you described what would it be? Then break it down and present it as convincingly as possible.

As mentioned, since this issue can never be considered done I'll close it but the discussion can continue here or elsewhere.

kvz commented 5 days ago

One small contribution from my side

not sure why the CDN bundle needs to be an npm module that we publish to npm. can the CDN bundle be installed from npm?

Not saying you should (i think you shouldn't, but yes I think you can do, e.g.: https://cdn.jsdelivr.net/npm/uppy@4.7.0/dist/uppy.min.js

Overall I think there are good and actionable insights in here. What would be the top ranking actionable thing we could do you think Mikael? Then we can turn that into an issue. Take it one by one until we reach the point where we collectively feel things left are too high hanging fruits.

These seem very painful in particular for any meaningful dev velocity and feel like maybe could use a dash of Remco doing another iteration on the monorepo

having to wait 30-60 sec for intellisense to appear loading every time i change some code. i have never experienced this in other projects of similar size trying to debug typescript issues with 100+ tsconfig files

mifi commented 5 days ago

If I have to choose only one, then I think #5379 is probably the one that gives the most value for users and for us, but that is a huge task.

Other than that, I think are the ones that require the least effort are the removal of the packages @uppy/aws-s3 @uppy/remote-sources @uppy/store-default @uppy/utils @uppy/locales uppy @uppy/redux-store @uppy/redux-dev-tools @uppy/svelte @uppy/angular @uppy/vue @uppy/react (or a subset of these that don't require much effort)

and maybe see if we can reduce the number of tsconfig files to additionally speed up typescript. do we need two sets of files for each plugin (tsconfig.build.json and tsconfig.json)?

Murderlon commented 5 days ago

If I have to choose only one

I meant pick one that doesn't have an issue yet. Something extra you'd like to see happen.


Regarding moving packages, I still don't see how it improves the situation. We still end up with the same amount of code? I would like to see additional benefits described, such as with #4936. If you see tangible benefits here, perhaps pitch this in a new issue?

Using the lord's editor (Neovim) all my TS suggestions are instant so we'd have to look deeper to see what really causes this for you.

Regarding tsconfig files, we should keep two tsconfig's per package. One of the reasons our monorepo is slow and complex is precisely because we tried to do everything from the top instead of per package. Settings are also different per package, such as react, vue, svelte, angular, react native, etc. It's best practice to have two tsconfig files per package.

mifi commented 5 days ago

Regarding moving packages, I still don't see how it improves the situation. We still end up with the same amount of code? I would like to see additional benefits described, such as with #4936. If you see tangible benefits here, perhaps pitch this in a new issue?

for those that we remove, obviously we will end up with less code to maintain. for those that we merge into core, we will end up with a simpler codebase with less boilerplate and less build steps. I tried to argue why I think the codebase should be simplified in this issue - I don't have time to make a proper issue/pitch now, but I will see if I can debug my intellisense experience, as it's probably something with my setup, if it's fast for you.

It's best practice to have two tsconfig files per package

you have a source for this?

Murderlon commented 5 days ago

you have a source for this?

We need to be able to make a distinction for what we build and publish to npm (the source) and type checking non-source files such as tests. For instance, as we've done here https://github.com/tus/tus-node-server/pull/641