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-multipart: pass the uploadURL back to the caller #4614

Merged
merged 5 commits into from
Aug 14, 2023
Merged

@uppy/aws-s3-multipart: pass the uploadURL back to the caller #4614

merged 5 commits into from
Aug 14, 2023

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Aug 8, 2023

Fixes: #4613

@aduh95 aduh95 mentioned this pull request Aug 8, 2023
Merged
@aduh95 aduh95 requested a review from Murderlon August 8, 2023 10:12
@Murderlon
Copy link
Member

Ooh CI is failing

@aduh95 aduh95 added the safe to test Add this label on trustworthy PRs to spawn the e2e test suite label Aug 8, 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 Aug 8, 2023
arturi
arturi reviewed Aug 9, 2023
View reviewed changes
packages/@uppy/aws-s3-multipart/src/index.js
if (method === 'POST' && location === null) {
// Not being able to read the Location header is not a fatal error.
// eslint-disable-next-line no-console
console.warn('AwsS3/Multipart: Could not read the Location header. This likely means CORS is not configured correctly on the S3 Bucket. See https://uppy.io/docs/aws-s3-multipart#S3-Bucket-Configuration for instructions.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this not uppy.log(msg, 'warn') instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uppy is not defined in this context (it's a static method, so this.uppy won't work either).

@Murderlon Murderlon mentioned this pull request Aug 14, 2023
Closed
2 tasks
arturi
arturi reviewed Aug 14, 2023
View reviewed changes
packages/@uppy/aws-s3-multipart/src/index.js Outdated

if (method === 'POST' && location === null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (method === 'POST' && location === null) {
if (method?.toLowerCase() === 'POST'.toLowerCase() && location === null) {

'post' === 'POST' = false

Copy link
Contributor

Choose a reason for hiding this comment

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

== instead of === on the location null check?

@arturi
Copy link
Contributor

arturi commented Aug 14, 2023

For me, with or without this PR, uploadURL is undefined in non-Multipart and defined in Multipart:

image

uppyDashboard.on('upload-success', (file, data) => {
    console.log(file)
    console.log(data)
  })

Looks like CORS, right? As the error message should have told me.

@arturi arturi merged commit 0ac6478 into transloadit:main Aug 14, 2023
@github-actions github-actions bot mentioned this pull request Aug 15, 2023
Merged
github-actions bot added a commit that referenced this pull request Aug 15, 2023
@github-actions
Release: uppy@3.14.0 (#4626)
cf84af7 
| Package                   | Version | Package                   | Version |
| ------------------------- | ------- | ------------------------- | ------- |
| @uppy/audio               |   1.1.2 | @uppy/locales             |   3.3.0 |
| @uppy/aws-s3              |   3.2.2 | @uppy/onedrive            |   3.1.3 |
| @uppy/aws-s3-multipart    |   3.5.3 | @uppy/progress-bar        |   3.0.3 |
| @uppy/box                 |   2.1.3 | @uppy/provider-views      |   3.5.0 |
| @uppy/companion           |   4.8.0 | @uppy/redux-dev-tools     |   3.0.3 |
| @uppy/companion-client    |   3.3.0 | @uppy/screen-capture      |   3.1.2 |
| @uppy/core                |   3.4.0 | @uppy/status-bar          |   3.2.4 |
| @uppy/dashboard           |   3.5.1 | @uppy/thumbnail-generator |   3.0.4 |
| @uppy/drag-drop           |   3.0.3 | @uppy/transloadit         |   3.2.1 |
| @uppy/dropbox             |   3.1.3 | @uppy/tus                 |   3.1.3 |
| @uppy/facebook            |   3.1.2 | @uppy/unsplash            |   3.2.2 |
| @uppy/file-input          |   3.0.3 | @uppy/url                 |   3.3.3 |
| @uppy/google-drive        |   3.2.1 | @uppy/webcam              |   3.3.2 |
| @uppy/image-editor        |   2.1.3 | @uppy/xhr-upload          |   3.3.2 |
| @uppy/informer            |   3.0.3 | @uppy/zoom                |   2.1.2 |
| @uppy/instagram           |   3.1.2 | uppy                      |  3.14.0 |

- meta: Readme improvements (Artur Paikin / #4622)
- @uppy/companion: Fix typos and add env vars to .env.example (Dominik Schmidt / #4624)
- @uppy/aws-s3-multipart: pass the `uploadURL` back to the caller (Antoine du Hamel / #4614)
- meta: update to node-18.17.0-alpine,  (odselsevier / #4617)
- @uppy/aws-s3,@uppy/aws-s3-multipart: update types (Antoine du Hamel / #4611)
- @uppy/aws-s3-multipart,@uppy/aws-s3,@uppy/companion,@uppy/transloadit,@uppy/xhr-upload: use uppercase HTTP method names (Antoine du Hamel / #4612)
- meta: e2e: fix race condition in transloadit test (Antoine du Hamel / #4616)
- @uppy/aws-s3,@uppy/aws-s3-multipart: update types (bdirito / #4576)
- @uppy/core: allow duplicate files with onBeforeFileAdded (Merlijn Vos / #4594)
- @uppy/companion: make CSRF protection helpers available to providers (Dominik Schmidt / #4554)
- @uppy/companion: fix Redis key default TTL (Subha Sarkar / #4607)
- @uppy/companion: Fix Uploader.js metadata normalisation (Subha Sarkar / #4608)
- @uppy/companion-client,@uppy/provider-views: make authentication optional (Dominik Schmidt / #4556)
- @uppy/provider-views: fix ProviderView error on empty plugin.icon (Dominik Schmidt / #4553)
- @uppy/aws-s3,@uppy/tus,@uppy/xhr-upload:  Invoke headers function for remote uploads (Dominik Schmidt / #4596)
- @uppy/companion: Unify redis initialization (Dominik Schmidt / #4597)
- meta: lock node-js version on ci (Mikael Finstad / #4606)
- @uppy/companion: allow dynamic S3 bucket (rmoura-92 / #4579)
- @uppy/status-bar: e2e: add test for retrying and pausing uploads (Antoine du Hamel / #3599)
- meta: e2e: remove too short timeout (Antoine du Hamel / #4602)
Murderlon added a commit that referenced this pull request Aug 24, 2023
@Murderlon
Merge branch 'main' into remote-upload-refactor
d47c0ee 
* main: (28 commits)
  Release: uppy@3.14.1 (#4644)
  @uppy/aws-s3-multipart: fix types when using deprecated option (#4634)
  @uppy/companion: harden lint rules (#4641)
  allow empty objects for `fields` types (#4631)
  meta: upgrade Node.js docker version (#4630)
  Release: uppy@3.14.0 (#4626)
  Readme improvements (#4622)
  Fix typos and add env vars to .env.example (#4624)
  @uppy/aws-s3-multipart: pass the `uploadURL` back to the caller (#4614)
  update to node-18.17.0-alpine,  (#4617)
  @uppy/aws-s3,@uppy/aws-s3-multipart: update types (#4611)
  use uppercase HTTP method names (#4612)
  e2e: fix race condition in transloadit test (#4616)
  @uppy/aws-s3,@uppy/aws-s3-multipart: update types (#4576)
  @uppy/core: allow duplicate files with onBeforeFileAdded (#4594)
  @uppy/companion: make CSRF protection helpers available to providers (#4554)
  @uppy/companion: fix Redis key default TTL (#4607)
  @uppy/companion: Fix Uploader.js metadata normalisation (#4608)
  @uppy/companion-client,@uppy/provider-views: make authentication optional (#4556)
  @uppy/provider-views: fix ProviderView error on empty plugin.icon (#4553)
  ...
@arturi arturi mentioned this pull request Sep 22, 2023
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arturi arturi arturi left review comments

@mifi mifi mifi left review comments

@Murderlon Murderlon Murderlon approved these changes

Assignees
No one assigned
Labels
pending end-to-end tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

data.uploadURL undefined in "upload-success" event, uploading images to S3 using @uppy/aws-s3
4 participants
@aduh95 @Murderlon @arturi @mifi