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: use Promise.allSettled instead of custom utils #3079

Merged
merged 2 commits into from
Nov 8, 2021
Merged

@uppy/aws-s3: use Promise.allSettled instead of custom utils #3079

merged 2 commits into from
Nov 8, 2021

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Aug 6, 2021 •
edited
Loading

This is a breaking change, I think we should land this before releasing 2.0.0 the method is now private, it's no longer a breaking change.

This PR also allows those promises to actually reject, as currently it would always fulfilled as there was a .catch call that didn't throw back the error.

@aduh95 aduh95 added the 2.0 label Aug 6, 2021
@goto-bus-stop
Copy link
Contributor

it not returning the value is intentional, the handleUpload() function should not reject if a single file fails to upload. later plugins do not run if that promise rejects.

@aduh95
Copy link
Contributor Author

aduh95 commented Aug 9, 2021

it not returning the value is intentional, the handleUpload() function should not reject if a single file fails to upload. later plugins do not run if that promise rejects.

OK so using Promise.all would make the most sense here, right? Let me address that in a follow up commit.

@aduh95
Copy link
Contributor Author

aduh95 commented Aug 9, 2021

Actually, given that Promise.allSettled doesn't reject even if a promise rejects, handleUpload cannot return a rejected promise with the current PR implementation.

@goto-bus-stop
Copy link
Contributor

oh yeah, the new Promise.reject() line is inside the allSettled() call. That's fine then, it just doesn't really change anything

@aduh95 aduh95 added Uppy 3.x and removed 2.0 labels Sep 28, 2021
@arturi
Copy link
Contributor

arturi commented Nov 8, 2021

Can we merge this @aduh95?

goto-bus-stop
goto-bus-stop approved these changes Nov 8, 2021
View reviewed changes
Copy link
Contributor

@goto-bus-stop goto-bus-stop left a comment

Choose a reason for hiding this comment

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

i will also make my "that's fine then" official 😇

@aduh95
Copy link
Contributor Author

aduh95 commented Nov 8, 2021

I thought it was a breaking change because this method used to be public, but because it's now a private method we can safely merge it I think :)

@aduh95 aduh95 removed the Uppy 3.x label Nov 8, 2021
@aduh95 aduh95 merged commit bc67c5e into transloadit:main Nov 8, 2021
@aduh95 aduh95 deleted the promise-allsettled-asw-s3 branch November 8, 2021 10:23
Murderlon added a commit that referenced this pull request Nov 8, 2021
@Murderlon
Merge branch 'main' into status-error-state
c1e2296 
* main:
  @uppy/aws-s3: use `Promise.allSettled` insead of custom utils (#3079)
  Convert sets to arrays prior to using spread operator to enable transpilation to ES5 (#3297)
  Add showRecordingLength to webcam types (#3303)
  Required meta fields UI (#3285)
  Update BACKLOG.md
  Release [email protected]
  @uppy/[email protected]
Murderlon added a commit that referenced this pull request Nov 10, 2021
@Murderlon
Merge branch 'main' into locale-docs
725a277 
* main:
  Remove references of incorrect `options` argument for `companion.socket` (#3307)
  Upgrade linting to 2.0.0-0 (#3280)
  Release [email protected], @uppy/[email protected]
  @uppy/[email protected]
  meta: update version references in docs
  Release @uppy/[email protected]
  @uppy/[email protected]
  @uppy/aws-s3: use `Promise.allSettled` insead of custom utils (#3079)
  Convert sets to arrays prior to using spread operator to enable transpilation to ES5 (#3297)
  Add showRecordingLength to webcam types (#3303)
  Required meta fields UI (#3285)
  Update BACKLOG.md
  Release [email protected]
  @uppy/[email protected]
  fix: @uppy/utils - `getFileType()` always returns a string (#3294)
  Remove yarn file
  @uppy/companion: fix Dockerfiles (#3291)
  @uppy/companion: use Node.js 16.x for Dockerfile (#3289)
  if an item is checked, it can’t be disabled (#3288)
Murderlon added a commit that referenced this pull request Nov 11, 2021
@Murderlon
Merge branch 'main' into tus-retry-fix
0f1d625 
* main: (30 commits)
  Refactor locale scripts & generate types and docs (#3276)
  Remove references of incorrect `options` argument for `companion.socket` (#3307)
  Upgrade linting to 2.0.0-0 (#3280)
  Release [email protected], @uppy/[email protected]
  @uppy/[email protected]
  meta: update version references in docs
  Release @uppy/[email protected]
  @uppy/[email protected]
  @uppy/aws-s3: use `Promise.allSettled` insead of custom utils (#3079)
  Convert sets to arrays prior to using spread operator to enable transpilation to ES5 (#3297)
  Add showRecordingLength to webcam types (#3303)
  Required meta fields UI (#3285)
  Update BACKLOG.md
  Release [email protected]
  @uppy/[email protected]
  fix: @uppy/utils - `getFileType()` always returns a string (#3294)
  Remove yarn file
  @uppy/companion: fix Dockerfiles (#3291)
  @uppy/companion: use Node.js 16.x for Dockerfile (#3289)
  if an item is checked, it can’t be disabled (#3288)
  ...
vymao pushed a commit to vymao/uppy that referenced this pull request Mar 29, 2022
@Murderlon
Merge branch 'main' into tus-retry-fix
7594272 
* main: (30 commits)
  Refactor locale scripts & generate types and docs (transloadit#3276)
  Remove references of incorrect `options` argument for `companion.socket` (transloadit#3307)
  Upgrade linting to 2.0.0-0 (transloadit#3280)
  Release [email protected], @uppy/[email protected]
  @uppy/[email protected]
  meta: update version references in docs
  Release @uppy/[email protected]
  @uppy/[email protected]
  @uppy/aws-s3: use `Promise.allSettled` insead of custom utils (transloadit#3079)
  Convert sets to arrays prior to using spread operator to enable transpilation to ES5 (transloadit#3297)
  Add showRecordingLength to webcam types (transloadit#3303)
  Required meta fields UI (transloadit#3285)
  Update BACKLOG.md
  Release [email protected]
  @uppy/[email protected]
  fix: @uppy/utils - `getFileType()` always returns a string (transloadit#3294)
  Remove yarn file
  @uppy/companion: fix Dockerfiles (transloadit#3291)
  @uppy/companion: use Node.js 16.x for Dockerfile (transloadit#3289)
  if an item is checked, it can’t be disabled (transloadit#3288)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goto-bus-stop goto-bus-stop goto-bus-stop approved these changes

@Murderlon Murderlon Murderlon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aduh95 @goto-bus-stop @arturi @Murderlon