@uppy/companion-client: avoid unnecessary preflight requests #4462

aduh95 · 2023-05-18T18:21:57Z

According to https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#simple_requests, we have so many preflight requests because we add Uppy-Versions header and Content-Type: application/json to each request. It's particularly not useful for for requests with no body / an empty body because the remote Companion doesn't check Content-Type.

It was added in #1612, but render moot by #2967.

arturi · 2023-05-19T16:26:54Z

Very nice find! Could this lead to compatibility issues since Uppy-Versions was added to keep Companion compatible with old versions of Uppy Client? It was long ago, could be that it's fine to remove that compat check, but it exists in Companion AFAIK: if new Uppy version, then send certain allowed response, if Uppy-Version is missing, don't. Could be fine for empty requests, like you mentioned?

aduh95 · 2023-05-22T16:15:30Z

Trying to look for Uppy-Versions or @uppy/companion-client= in the codebase doesn't return any result, are you sure it's still in used?

EDIT: found it:

uppy/packages/@uppy/companion/src/server/middlewares.js

Line 194 in 596989d

clientVersion: req.header('uppy-versions') || versionFromQuery || '1.0.0',

mifi · 2023-06-04T21:46:50Z

Not sure what it's even being used for though.

it seems to be stored in oauth state here:

uppy/packages/@uppy/companion/src/server/controllers/connect.js

Line 23 in 0e2618b

    
           state = oAuthState.addToState(state, { clientVersion: req.companion.clientVersion }, secret)

and logged here:

uppy/packages/@uppy/companion/src/server/middlewares.js

Line 199 in 0e2618b

    
           logger.info(`uppy client version ${req.companion.clientVersion}`, 'companion.client.version')

but other than that I cannot find it being used anywhere. maybe it can be removed?

mifi

nice

packages/@uppy/companion-client/src/RequestClient.js

mifi · 2023-11-02T11:04:21Z

so i guess it boils down to:

are we willing to take the risk and remove uppy-versions for requests without a body (possibly breaking old companion versions)?
if so, should we just remove uppy-versions for all requests for consistency?

mifi · 2023-12-04T13:52:11Z

was added here d77180a - it seesm to not be needed anymore, so we can remove it. let's also remove all the other code from that commit

mifi · 2023-12-05T08:08:39Z

i'll take over this then

from d77180a including removing the `logClientVersion` option and custom preflight logic

see https://vitest.dev/guide/features#watch-mode

mifi · 2023-12-05T08:41:57Z

Now that I think more about it and look at some old threads #2855 #2610 (comment) - maybe it's a breaking change after all? if some people depend on the uppy client version being logged with the logClientVersion option?

Murderlon · 2023-12-05T08:44:58Z

But the presence/absence of a log is not breaking right? The option would just be silently ignored. I think that's fine.

mifi · 2023-12-05T08:47:23Z

maybe not 😇

packages/@uppy/companion-client/src/RequestClient.js

packages/@uppy/companion/src/config/companion.js

packages/@uppy/companion/src/server/controllers/connect.js

mifi · 2023-12-05T11:08:33Z

how do you run eslint --fix in github @aduh95 ?

packages/@uppy/companion-client/src/RequestClient.js

Co-authored-by: Mikael Finstad <[email protected]>

aduh95 · 2023-12-05T11:11:07Z

how do you run eslint --fix in github @aduh95 ?

I don't think that's a thing.

package.json

mifi · 2023-12-05T11:38:11Z

Oh i just assumed you didnt do all those by hand 😅

…-core * 'ts-core' of https://github.com/transloadit/uppy: prettier Apply suggestion from code reviews fix lint Fix unit tests meta: fix `js2ts` script on Node.js 20+ (#4802) @uppy/companion-client: avoid unnecessary preflight requests (#4462)

| Package | Version | Package | Version | | ---------------------- | ------- | ---------------------- | ------- | | @uppy/aws-s3 | 3.6.0 | @uppy/instagram | 3.2.0 | | @uppy/aws-s3-multipart | 3.10.0 | @uppy/onedrive | 3.2.0 | | @uppy/box | 2.2.0 | @uppy/provider-views | 3.8.0 | | @uppy/companion | 4.12.0 | @uppy/store-default | 3.2.0 | | @uppy/companion-client | 3.7.0 | @uppy/tus | 3.5.0 | | @uppy/core | 3.8.0 | @uppy/url | 3.5.0 | | @uppy/dropbox | 3.2.0 | @uppy/utils | 5.7.0 | | @uppy/facebook | 3.2.0 | @uppy/xhr-upload | 3.6.0 | | @uppy/google-drive | 3.4.0 | @uppy/zoom | 2.2.0 | | @uppy/image-editor | 2.4.0 | uppy | 3.21.0 | - @uppy/provider-views: fix uploadRemoteFile undefined (Mikael Finstad / #4814) - @uppy/companion: fix double tus uploads (Mikael Finstad / #4816) - @uppy/companion: fix accelerated endpoints for presigned POST (Mikael Finstad / #4817) - @uppy/companion: fix `authProvider` property inconsistency (Mikael Finstad / #4672) - @uppy/companion: send certain onedrive errors to the user (Mikael Finstad / #4671) - meta: fix typo in `lockfile_check.yml` name (Antoine du Hamel) - @uppy/aws-s3: change Companion URL in tests (Antoine du Hamel) - @uppy/set-state: fix types (Antoine du Hamel) - @uppy/companion: Provider user sessions (Mikael Finstad / #4619) - meta: fix `js2ts` script on Node.js 20+ (Merlijn Vos / #4802) - @uppy/companion-client: avoid unnecessary preflight requests (Antoine du Hamel / #4462) - meta: Migrate to AWS-SDK V3 syntax (Artur Paikin / #4810) - @uppy/utils: fix import in test files (Antoine du Hamel / #4806) - @uppy/core: Fix onBeforeFileAdded with Golden Retriever (Merlijn Vos / #4799) - @uppy/image-editor: respect `cropperOptions.initialAspectRatio` (Lucklj521 / #4805)

@uppy/companion-client: avoid unecessary preflight requests

a6b5aaa

github-actions bot added the pending end-to-end tests label May 18, 2023

arturi requested a review from mifi May 19, 2023 16:24

mifi reviewed Jun 4, 2023

View reviewed changes

packages/@uppy/companion-client/src/RequestClient.js Outdated Show resolved Hide resolved

arturi assigned aduh95 Jun 6, 2023

aduh95 mentioned this pull request Jun 22, 2023

examples/aws-nodejs: showcase an example without preflight requests #4516

Merged

Merge branch 'main' into pr/4462

99e4a55

mifi added 2 commits December 5, 2023 16:39

reverse all changes...

e0e2d4a

from d77180a including removing the `logClientVersion` option and custom preflight logic

fix yarn test

ca5a315

see https://vitest.dev/guide/features#watch-mode

Murderlon approved these changes Dec 5, 2023

View reviewed changes

aduh95 commented Dec 5, 2023

View reviewed changes

Remove unrelated whitespace changes

1c9aea5

aduh95 commented Dec 5, 2023

View reviewed changes

packages/@uppy/companion-client/src/RequestClient.js Outdated Show resolved Hide resolved

s/emptyRequest/emptyBody/

343c2ac

Co-authored-by: Mikael Finstad <[email protected]>

aduh95 commented Dec 5, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

Do not set CI=true on non-CI env

ab5b81c

aduh95 merged commit f55b24c into transloadit:main Dec 5, 2023
19 checks passed

aduh95 deleted the fewer-preflight branch December 5, 2023 11:30

github-actions bot mentioned this pull request Dec 12, 2023

Release: [email protected] #4823

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

@uppy/companion-client: avoid unnecessary preflight requests #4462

@uppy/companion-client: avoid unnecessary preflight requests #4462

aduh95 commented May 18, 2023 •

edited

Loading

arturi commented May 19, 2023

aduh95 commented May 22, 2023 •

edited

Loading

mifi commented Jun 4, 2023

mifi left a comment

mifi commented Nov 2, 2023

mifi commented Dec 4, 2023 •

edited

Loading

mifi commented Dec 5, 2023

mifi commented Dec 5, 2023

Murderlon commented Dec 5, 2023

mifi commented Dec 5, 2023

mifi commented Dec 5, 2023

aduh95 commented Dec 5, 2023 •

edited

Loading

mifi commented Dec 5, 2023

@uppy/companion-client: avoid unnecessary preflight requests #4462

@uppy/companion-client: avoid unnecessary preflight requests #4462

Conversation

aduh95 commented May 18, 2023 • edited Loading

arturi commented May 19, 2023

aduh95 commented May 22, 2023 • edited Loading

mifi commented Jun 4, 2023

mifi left a comment

Choose a reason for hiding this comment

mifi commented Nov 2, 2023

mifi commented Dec 4, 2023 • edited Loading

mifi commented Dec 5, 2023

mifi commented Dec 5, 2023

Murderlon commented Dec 5, 2023

mifi commented Dec 5, 2023

mifi commented Dec 5, 2023

aduh95 commented Dec 5, 2023 • edited Loading

mifi commented Dec 5, 2023

aduh95 commented May 18, 2023 •

edited

Loading

aduh95 commented May 22, 2023 •

edited

Loading

mifi commented Dec 4, 2023 •

edited

Loading

aduh95 commented Dec 5, 2023 •

edited

Loading