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.

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

Invoke headers function for remote uploads #4596

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

dschmidt
Copy link
Contributor

@dschmidt dschmidt commented Jul 20, 2023

Streamline the behavior of local and remote uploads - currently when headers is a function it's serialized to undefined (or rather it's skipped) for remote uploads but for local uploads it's called and the result is serialized.

c.f.

if (typeof opts.headers === 'function') {
opts.headers = opts.headers(file)
}

if (typeof headers === 'function') {
opts.headers = headers(file)
} else {
Object.assign(opts.headers, this.opts.headers)
}

There is also

if (headers) {
xhrOpts.headers = headers
}

Which looks like it should do the same, but the code is a little different and I'm not 100% sure what I'm doing there

@dschmidt dschmidt changed the title Invoke headers function for remote uploads Invoke headers function for all uploads Jul 20, 2023
@dschmidt dschmidt force-pushed the remote-headers-function branch from 4c536ba to 410171a Compare July 20, 2023 15:21
@dschmidt dschmidt changed the title Invoke headers function for all uploads Invoke headers function for remote uploads Jul 20, 2023
@arturi arturi requested a review from mifi July 21, 2023 14:58
@dschmidt dschmidt mentioned this pull request Jul 21, 2023
2 tasks
Copy link
Contributor

@mifi mifi left a comment

Choose a reason for hiding this comment

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

Nice catch! preferably we should only do this once (probably initially when setting and validating this.opts?) but the code is a bit messy so I guess we can do a refactor later. For now, should we create a helper function so we don't have to duplicate the logic?

@dschmidt
Copy link
Contributor Author

tbh I think it's so messy, that it needs a proper refactor and it's not worth adding a helper function

The logic is pretty simple and this way it's easier to directly see what's going on without jumping to the function

tl;dr: I would keep it as it is

@mifi mifi merged commit 54ad2e4 into transloadit:main Jul 31, 2023
@dschmidt dschmidt deleted the remote-headers-function branch July 31, 2023 19:58
@github-actions github-actions bot mentioned this pull request Aug 15, 2023
github-actions bot added a commit that referenced this pull request Aug 15, 2023
| 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants