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

Move remote file upload logic into companion-client #4573

Merged
merged 14 commits into from
Aug 24, 2023
Merged

Move remote file upload logic into companion-client #4573

merged 14 commits into from
Aug 24, 2023

Conversation

Murderlon
Copy link
Member

@Murderlon Murderlon commented Jul 13, 2023 •
edited
Loading

Closes #4377
Closes #4585

  • Remove UploaderPlugin
  • Move all shared remote file uploading logic into @uppy/companion-client.
  • Move shared events into EventManager

@Murderlon Murderlon requested review from mifi, arturi and aduh95 July 13, 2023 14:56
@aduh95 aduh95 added the 4.0 For the 4.0 major version label Jul 13, 2023
@Murderlon
Copy link
Member Author

@aduh95 I'd argue this is not a breaking change. UploaderPlugin was an internal quick hack, not something people should rely on and it's not documented. Other than that there are no breaking changes for the plugins.

@aduh95 aduh95 mentioned this pull request Jul 17, 2023
Merged
@Murderlon Murderlon self-assigned this Jul 18, 2023
@Murderlon Murderlon removed the 4.0 For the 4.0 major version label Jul 18, 2023
@aduh95 aduh95 linked an issue Jul 19, 2023 that may be closed by this pull request
Could not resolve "@uppy/core/lib/UploaderPlugin.js" #4585
Closed
2 tasks
@Murderlon Murderlon marked this pull request as draft July 24, 2023 07:46
Murderlon added 2 commits July 24, 2023 11:21
@Murderlon
Merge branch 'main' into remote-upload-refactor
e08eb7d 
* main:
  @uppy/aws-s3-multipart: refresh file before calling user-defined functions (#4557)
  Release: uppy@3.13.0 (#4595)
  Add i18n to CONTRIBUTING.md (#4591)
  Add VirtualList to ProviderView (#4566)
  @uppy/provider-views: fix race conditions with folder loading (#4578)
  @uppy/status-bar: fix ETA when status bar is installed during upload (#4588)
  @uppy/provider-views: fix infinite folder loading  (#4590)
  examples/aws: client-side signing (#4463)
  Bump word-wrap from 1.2.3 to 1.2.4 (#4586)
  e2e: increase `requestTimeout` to 16s (#4587)
  @uppy/locales: update zh_TW translation (#4583)
  @uppy/aws-s3-multipart: fix crash on pause/resume (#4581)
  @uppy/aws-s3-multipart: do not access `globalThis.crypto` on the top-level (#4584)
  Release: uppy@3.12.0 (#4574)
  @uppy/transloadit: fix error message (#4572)
  @uppy/provider-views: add support for remote file paths (#4537)
  @uppy/transloadit: implement Server-sent event API (#4098)
@Murderlon
Inject correct companion POST args into companion-client
baed2db
@Murderlon Murderlon marked this pull request as ready for review July 25, 2023 10:16
arturi
arturi reviewed Aug 22, 2023
View reviewed changes
private/dev/Dashboard.js
@@ -125,7 +125,7 @@ export default () => {
uppyDashboard.use(AwsS3Multipart, { companionUrl: COMPANION_URL, limit: 6 })
break
case 'xhr':
uppyDashboard.use(XHRUpload, { endpoint: XHR_ENDPOINT, limit: 6, bundle: true })
uppyDashboard.use(XHRUpload, { endpoint: XHR_ENDPOINT, limit: 6, bundle: false })
Copy link
Contributor

Choose a reason for hiding this comment

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

Intentional? I don't mind, just making sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Remote uploading doesn't work without this change. Probably better to have it like this in dev.

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean pause/resume doesn't? Uploading seems to be working fine.

Copy link
Member Author

@Murderlon Murderlon Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

it must be false to work for remote files:

uppy/packages/@uppy/xhr-upload/src/index.js

Lines 641 to 646 in 7e48218

if (this.opts.bundle) {
// if bundle: true, we don’t support remote uploads
const isSomeFileRemote = filesFiltered.some(file => file.isRemote)
if (isSomeFileRemote) {
throw new Error('Can’t upload remote files when the `bundle: true` option is set')
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Aaaah right.

@arturi
Copy link
Contributor

arturi commented Aug 22, 2023

Good work! I tested with all uploaders (tus, xhr, s3 old and new, transloadit) and remote uploads, pause-resume in tus, cancel in others — didn't crash anywhere so far. Might have missed something, we'll have to fix as we go along then.

@Murderlon
Simplify
66c714d
@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)
  ...
@Murderlon
Revert "Simplify"
e4260d4 
This reverts commit 66c714d.
@Murderlon
Conditional requestclient vs provider was needed after all
8aa57cf
@Murderlon Murderlon merged commit ef613e6 into main Aug 24, 2023
@Murderlon Murderlon deleted the remote-upload-refactor branch August 24, 2023 12:27
This was referenced Sep 5, 2023
Closed
Merged
github-actions bot added a commit that referenced this pull request Sep 5, 2023
@github-actions
Release: uppy@3.15.0 (#4660)
a16de33 
| Package                | Version | Package                | Version |
| ---------------------- | ------- | ---------------------- | ------- |
| @uppy/angular          |   0.6.0 | @uppy/dashboard        |   3.5.2 |
| @uppy/aws-s3           |   3.3.0 | @uppy/transloadit      |   3.3.0 |
| @uppy/aws-s3-multipart |   3.6.0 | @uppy/tus              |   3.2.0 |
| @uppy/companion        |   4.8.2 | @uppy/utils            |   5.5.0 |
| @uppy/companion-client |   3.4.0 | @uppy/xhr-upload       |   3.4.0 |
| @uppy/core             |   3.5.0 | uppy                   |  3.15.0 |

- @uppy/transloadit: Emit assembly progress events (Marius / #4603)
- @uppy/transloadit: remove Socket.io (Antoine du Hamel / #4281)
- meta: example: update Angular example to 16.x (Antoine du Hamel / #4642)
- @uppy/angular: upgrade to Angular 16.x (Antoine du Hamel / #4642)
- @uppy/companion: refactor `getProtectedHttpAgent` to make TS happy (Antoine du Hamel / #4654)
- @uppy/companion: Alias "removeListener" as "off" in Redis emitter (Elliot Dickison / #4647)
- @uppy/aws-s3-multipart,@uppy/aws-s3,@uppy/companion-client,@uppy/core,@uppy/tus,@uppy/utils,@uppy/xhr-upload: Move remote file upload logic into companion-client (Merlijn Vos / #4573)
- @uppy/dashboard: when showAddFilesPanel  is true, aria-hidden should be the opposite (Artur Paikin / #4643)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arturi arturi arturi approved these changes

@aduh95 aduh95 aduh95 approved these changes

@mifi mifi Awaiting requested review from mifi

Assignees

@Murderlon Murderlon

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Could not resolve "@uppy/core/lib/UploaderPlugin.js" Reduce code duplication
3 participants
@Murderlon @arturi @aduh95