Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

@uppy/aws-s3: add shouldUseMultipart option #4299

Merged
merged 4 commits into from
Jun 19, 2023
Merged

@uppy/aws-s3: add shouldUseMultipart option #4299

merged 4 commits into from
Jun 19, 2023

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Feb 2, 2023

The shouldUseMultipart option is a way for the user to opt-in to mix multipart/non-multipart upload. When the option is set to something that's not false, the @uppy/aws-s3-multipart will be used instead, a future version of Uppy will merge both plugins into one.

Blocked on #4205.

@aduh95 aduh95 marked this pull request as ready for review April 26, 2023 13:56
@aduh95 aduh95 added the safe to test Add this label on trustworthy PRs to spawn the e2e test suite label Apr 26, 2023
@github-actions github-actions bot removed pending end-to-end tests safe to test Add this label on trustworthy PRs to spawn the e2e test suite labels Apr 26, 2023
@aduh95 aduh95 mentioned this pull request Apr 26, 2023
Merged
Murderlon
Murderlon requested changes Apr 28, 2023
View reviewed changes
Copy link
Member

@Murderlon Murderlon left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the point of adding this? It only makes sense to do it conditionally, but this is always or never, which is the same as just installing the other plugin? Aren't we needlessly increasing the bundle size?

@arturi
Copy link
Contributor

arturi commented May 2, 2023

We discussed the increase in size with Antoine and decided it's probably worth it (30kb minified), but I missed the point that per-file conditions won't be applied here indeed? Then it's probably not worth it.

So: we tell users who want new conditions to use @uppy/aws-s3-multipart, and then in the next major @uppy/aws-s3-multipart is basically renamed to @uppy/aws-s3, and that's it.

@Murderlon
Copy link
Member

Murderlon commented May 3, 2023 •
edited
Loading

Yes this temporary solution here only creates more confusion. Both plugins have a shouldUseMultipart, but one works per file and the other as a permanent switch? Even if they both behaved the same I think we shouldn't add it to both plugins because that's still confusing for people

@aduh95
Copy link
Contributor Author

aduh95 commented May 3, 2023

Both options behave exactly the same: either per file or as a permanent switch. When provided, aws-s3 plugin would behave exactly as if it was aws-s3-multipart, allowing folks to start using the new behavior without having to rewrite all their imports.

@arturi
Copy link
Contributor

arturi commented May 3, 2023

Ah right, it returns the new Multipart, but it then gets the same option, so it does pass the per-file conditions to it. Sorry, it should be clear enough by now, but even after working with it and testing both plugins with/without options I still was confused in my comment above.

Murderlon
Murderlon approved these changes May 3, 2023
View reviewed changes
Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless we are doing a major soon, I don't think it makes sense to make this package quite a bit heavier if the switching functionality is already in multipart. That would leave users with a heavier package with no gains for them. But if majority agrees then code LGTM

@aduh95
Copy link
Contributor Author

aduh95 commented May 3, 2023

FWIW, I agree there’s no rush, we probably want to wait a bit to be sure the feature is stable, and to have a blog post ready to explain what are our plans for the S3 plugin(s).

@aduh95 aduh95 added the safe to test Add this label on trustworthy PRs to spawn the e2e test suite label May 24, 2023
@github-actions github-actions bot removed pending end-to-end tests safe to test Add this label on trustworthy PRs to spawn the e2e test suite labels May 24, 2023
@aduh95 aduh95 merged commit c687d85 into transloadit:main Jun 19, 2023
@aduh95 aduh95 deleted the aws-shouldUseMultipart branch June 19, 2023 14:25
@github-actions github-actions bot mentioned this pull request Jun 19, 2023
Merged
github-actions bot added a commit that referenced this pull request Jun 19, 2023
@github-actions
Release: uppy@3.10.0 (#4511)
147e5da 
| Package                | Version | Package                | Version |
| ---------------------- | ------- | ---------------------- | ------- |
| @uppy/aws-s3           |   3.2.0 | @uppy/status-bar       |   3.2.0 |
| @uppy/aws-s3-multipart |   3.4.0 | @uppy/transloadit      |   3.1.6 |
| @uppy/companion        |   4.5.1 | @uppy/tus              |   3.1.1 |
| @uppy/core             |   3.2.1 | @uppy/url              |   3.3.2 |
| @uppy/dashboard        |   3.4.1 | @uppy/utils            |   5.4.0 |
| @uppy/golden-retriever |   3.0.4 | @uppy/xhr-upload       |   3.3.0 |
| @uppy/locales          |   3.2.2 | uppy                   |  3.10.0 |
| @uppy/provider-views   |   3.3.1 |                        |         |

- @uppy/aws-s3-multipart: fix the chunk size calculation (Antoine du Hamel / #4508)
- @uppy/aws-s3: add `shouldUseMultipart` option (Antoine du Hamel / #4299)
- @uppy/companion: switch from aws-sdk v2 to @aws-sdk/* (v3) (Scott Bessler / #4285)
- @uppy/companion,@uppy/core,@uppy/dashboard,@uppy/golden-retriever,@uppy/status-bar,@uppy/utils: Migrate all lodash' per-method-packages usage to lodash. (LinusMain / #4274)
- @uppy/core: Don't set late (throttled) progress event on a file that is 100% complete (Artur Paikin / #4507)
- @uppy/companion: revert randomness from file names (Mikael Finstad / #4509)
- @uppy/companion: Custom provider fixes (Mikael Finstad / #4498)
- @uppy/transloadit: ensure `fields` is not nullish when there no uploaded files (Antoine du Hamel / #4487)
- @uppy/aws-s3-multipart,@uppy/aws-s3,@uppy/tus,@uppy/utils,@uppy/xhr-upload: When file is removed (or all are canceled), controller.abort queued requests (Artur Paikin / #4504)
- @uppy/provider-views: Fix range selection not resetting and computing correctly (Terence C / #4415)
- meta: disallow use of `.only` in tests (Antoine du Hamel / #4494)
- @uppy/companion: fix 500 when file name contains non-ASCII chars (Antoine du Hamel / #4493)
- @uppy/locales: update `fr_FR.js` (Samuel De Backer / #4499)
- @uppy/aws-s3-multipart,@uppy/tus,@uppy/xhr-upload: Don't close socket while upload is still in progress (Artur Paikin / #4479)
- meta: bump `luxon` from 1.28.0 to 1.28.1 (dependabot[bot] / #4497)
- @uppy/utils: rename `EventTracker` -> `EventManager` (Stephen Wooten / #4481)
- meta: bump cookiejar from 2.1.3 to 2.1.4 (dependabot[bot] / #4496)
- meta: make `pre-commit` use `corepack yarn` instead of `npm run` (Antoine du Hamel / #4495)
- meta: bump ua-parser-js from 0.7.31 to 0.7.35 (dependabot[bot] / #4474)
- meta: bump @sideway/formula from 3.0.0 to 3.0.1 (dependabot[bot] / #4473)
- meta: bump http-cache-semantics from 4.1.0 to 4.1.1 (dependabot[bot] / #4472)
- @uppy/companion: Use filename from content-disposition instead of relying on url, with fallback (Artur Paikin / #4489)
- meta: bump `babel`, `esbuild`, and `vite` (dependabot[bot] / #4485)
- @uppy/dashboard: include the old state when setting new (Artur Paikin / #4490)
- @uppy/companion: fix companion implicitpath (Mikael Finstad / #4484)
- @uppy/companion: fix undefined protocol and example page (Mikael Finstad / #4483)
- meta: upgrade Cypress 12.9.0 -> 12.14.0 (Antoine du Hamel / #4491)
- @uppy/core: remove `state` getter from types (Antoine du Hamel / #4477)
- examples/php-xhr: Added filename sanitation and file size check before saving (neuronet77 / #4432)
- examples/php-xhr: update PHP dependencies (dependabot[bot])
- @uppy/xhr-upload: add support for arrays in metadata (Vasiliy Matyushin / #4431)
- @uppy/status-bar: Filtered ETA (stduhpf / #4458)
- @uppy/aws-s3-multipart: fix `getUploadParameters` option (Antoine du Hamel / #4465)
Murderlon added a commit that referenced this pull request Jun 20, 2023
@Murderlon
Merge branch 'main' into fix-throttle-global-state
438e6d6 
* main: (61 commits)
  Release: uppy@3.10.0 (#4511)
  @uppy/aws-s3-multipart: fix the chunk size calculation (#4508)
  @uppy/aws-s3: add `shouldUseMultipart` option (#4299)
  @uppy/companion: switch from aws-sdk v2 to @aws-sdk/* (v3) (#4285)
  Migrate all lodash' per-method-packages usage to lodash. (#4274)
  @uppy/core: Don't set late (throttled) progress event on a file that is 100% complete (#4507)
  @uppy/companion: revert randomness from file names (#4509)
  Custom provider fixes (#4498)
  @uppy/transloadit: ensure `fields` is not nullish when there no uploaded files (#4487)
  When file is removed (or all are canceled), controller.abort queued requests (#4504)
  @uppy/provider-views: Fix range selection not resetting and computing correctly (#4415)
  meta: disallow use of `.only` in tests (#4494)
  @uppy/companion: fix 500 when file name contains non-ASCII chars (#4493)
  @uppy/locales: update `fr_FR.js` (#4499)
  Don't close socket while upload is still in progress (#4479)
  meta: bump `luxon` from 1.28.0 to 1.28.1 (#4497)
  @uppy/utils: rename `EventTracker` -> `EventManager` (#4481)
  meta: bump cookiejar from 2.1.3 to 2.1.4 (#4496)
  meta: make `pre-commit` use `corepack yarn` instead of `npm run` (#4495)
  meta: bump ua-parser-js from 0.7.31 to 0.7.35 (#4474)
  ...
Murderlon added a commit that referenced this pull request Jun 20, 2023
@Murderlon
Merge branch 'main' into hide-pause-resume-remote
2e2afb7 
* main:
  Release: uppy@3.10.0 (#4511)
  @uppy/aws-s3-multipart: fix the chunk size calculation (#4508)
  @uppy/aws-s3: add `shouldUseMultipart` option (#4299)
  @uppy/companion: switch from aws-sdk v2 to @aws-sdk/* (v3) (#4285)
  Migrate all lodash' per-method-packages usage to lodash. (#4274)
  @uppy/core: Don't set late (throttled) progress event on a file that is 100% complete (#4507)
  @uppy/companion: revert randomness from file names (#4509)
  Custom provider fixes (#4498)
  @uppy/transloadit: ensure `fields` is not nullish when there no uploaded files (#4487)
  When file is removed (or all are canceled), controller.abort queued requests (#4504)
  @uppy/provider-views: Fix range selection not resetting and computing correctly (#4415)
  meta: disallow use of `.only` in tests (#4494)
@arturi arturi mentioned this pull request Aug 16, 2023
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mifi mifi mifi left review comments

@Thompson1985 Thompson1985 Thompson1985 approved these changes

@Murderlon Murderlon Murderlon approved these changes

Assignees

@arturi arturi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aduh95 @arturi @Murderlon @mifi @Thompson1985