companion,companion-client: send uppy-versions header to companion #1612

ifedapoolarewaju · 2019-05-29T15:25:48Z

No description provided.

kvz

Lgtm! Just some minor nitpicks

packages/@uppy/companion/test/__tests__/companion.js

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

kvz · 2019-05-30T08:41:17Z

I’m afraid I reviewed yesterday but forgot to hit the button so all my remarks where pending for the night :o Cool that it’s resolved! Sent from mobile, pardon the brevity.

…

On 30 May 2019, at 10:25, Ifedapo .A. Olarewaju ***@***.***> wrote: @ifedapoolarewaju commented on this pull request. In ***@***.***/companion-client/src/RequestClient.js: > @@ -75,9 +80,48 @@ module.exports = class RequestClient { return res.json() } + preflight (path) { + return new Promise((resolve, reject) => { + if (this.preflightDone) { + return resolve(this.allowedHeaders.slice()) + } + + fetch(this._getUrl(path), { + method: 'OPTIONS' + }) + .then((response) => { + if (response.headers.has('access-control-allow-headers')) { + const allowedHeaders = response.headers.get('access-control-allow-headers') + .split(',').map((headerName) => headerName.trim().toLowerCase()) + this.allowedHeaders = this.allowedHeaders.concat(allowedHeaders) I had changed this yesterday, already. Maybe you needed to do a pull/refresh? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

packages/@uppy/companion/tsconfig.json

packages/@uppy/companion/src/server/controllers/send-token.js

resolved

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

goto-bus-stop

functionality LGTM, imo the promise stuff does make a big difference here but i know you felt differently last time, so you can ignore my suggestion there if you like

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

packages/@uppy/provider-views/src/index.js

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

arturi · 2019-06-11T18:08:41Z

Still getting with uppy@1.0.1 (which doesn’t send version):

TypeError: Cannot read property 'version' of null
    at Object.exports.gte (/Users/artur/Projects/transloadit/uppy/packages/@uppy/companion/src/server/helpers/version.js:10:25)

Left a comment about the possible cause.

Please test with npm run web:start and http://localhost:4000 homepage, it still uses uppy@1.0.1 on the homepage in this PR.

ifedapoolarewaju · 2019-06-13T16:34:07Z

@arturi fixed, last time hopefully

goto-bus-stop · 2019-06-16T14:44:56Z

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

+      }
+
+      fetch(this._getUrl(path), {
+        method: 'OPTIONS'


I'm actually getting an error on this because OPTIONS is not in the CORS whitelist …

Request headers:

Response headers:

If this is the case we may need a new separate endpoint that we can just GET after all :(

I am unable to reproduce the issue locally with firefox. We've always allowed all methods for all endpoints

Are you using the companion standalone server when testing? Also are you able to reproduce this with other providers asides S3? Here's my network log when testing with instagram on firefox

Did renee test it against transloadit? Can you? Maybe we're messing something up over there?

Ahh…I tested using the examples/aws-companion example in this repo, which does not use the standalone server, and uses the default configuration of the cors module. It doesn't do this thing: https://github.com/expressjs/cors#enabling-cors-pre-flight

Did renee test it against transloadit? Can you? Maybe we're messing something up over there?

Transloadit directly uses the standalone server so I wouldn't think we would be messing anything up there.

Ahh…I tested using the examples/aws-companion example in this repo, which does not use the standalone server

Ah alright, it makes sense now.

Users who have their own personalized express servers are left to determine what methods they allow on their servers, but maybe we should change this and let the companion pluggable app set the allowed methods for its own endpoints 🤔

Also, should this still be considered a breaking change if it breaks only for users who don't use the standalone server, and happen not to allow the OPTIONS method on their server? 🤔

Right, the entire upload fails right now if the preflight request fails, maybe we can change it so the preflight request failure is caught, and we use the default allowedHeaders in that case?

Transloadit directly uses the standalone server so I wouldn't think we would be messing anything up there.

That's true but it does pass through HAProxy which does/can tamper with headers. Not the issue right now it seems, but I thought i'd add that as a friendly reminder here to save us a possible headache later.

That's true but it does pass through HAProxy which does/can tamper with headers

Ah ok, I thought you once mentioned that it does almost nothing (for the case of companion) but forward requests to companion, so I was working with that.

maybe we can change it so the preflight request failure is caught, and we use the default allowedHeaders in that case?

will do 👍

@goto-bus-stop I have made an update.

PS: I had to force push because I did a rebase to resolve conflicts

also encode versions sent through query params

goto-bus-stop

works for me! 👍

arturi · 2019-06-25T14:49:43Z

All good! The e2e test failure seems to be about package-lock only, probably be good on master?

ifedapoolarewaju · 2019-06-26T11:03:30Z

The e2e test failure seems to be about package-lock only, probably be good on master?

we'll see. Merging now.

davekiss · 2019-07-25T16:39:36Z

Just a note for others, I had to add the following headers to my preflight CORS response on a custom serverless Express instance in order to get rid of some breaking errors introduced by this PR

  res.header("Access-Control-Allow-Methods", "OPTIONS, GET, POST, PATCH, PUT");
  res.header("Access-Control-Allow-Credentials", "true");
  res.header("Access-Control-Expose-Headers", "Access-Control-Allow-Headers");

shell362 · 2019-08-18T14:34:41Z

@ifedapoolarewaju
Is there a way to disable CORS preflight at all? I don't need it, and my server doesn't support it.

ifedapoolarewaju · 2019-08-30T09:24:18Z

@Shoroh As seen, in this PR, subsequent versions of companion will support it internally, and there'll be no need to explicitly add this to your server.

shell362 · 2019-08-30T10:03:05Z

I don't use the companion, I create my own. I use only frontend Uppy client, and I don't want it to make OPTIONS requests.

ifedapoolarewaju · 2019-08-30T10:40:18Z

Hmm, then it gets tricky, because companion-client expects / assumes that it's communicating with companion.

Might I ask how your setup is? So you have a customer server that behaves similarly to companion, but isn't companion?

shell362 · 2019-08-30T11:27:14Z

you have a customer server that behaves similarly to companion, but isn't companion?

Exactly.
I use Ruby On Rails backend with "shrine" gem. The gem provides the similar to companion API for direct upload. Like, Uppy asks for the direct link, shrine returns one, everyone is happy.

Except for that thing Ruby On Rails doesn't support Options requests out of the box. And it's ok, cause I don't see any reason to support it to get the direct link.

ifedapoolarewaju requested review from goto-bus-stop, kvz and arturi May 29, 2019 15:25

ifedapoolarewaju force-pushed the preflight-version-header branch from 29f8bf7 to f1d23c3 Compare May 29, 2019 20:28

kvz previously requested changes May 30, 2019

View reviewed changes

packages/@uppy/companion/test/__tests__/companion.js Outdated Show resolved Hide resolved

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

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

arturi reviewed May 30, 2019

View reviewed changes

packages/@uppy/companion/tsconfig.json Outdated Show resolved Hide resolved

arturi reviewed May 30, 2019

View reviewed changes

packages/@uppy/companion/src/server/controllers/send-token.js Show resolved Hide resolved

ifedapoolarewaju force-pushed the preflight-version-header branch from 6443367 to 93e0e5f Compare June 5, 2019 10:26

goto-bus-stop reviewed Jun 5, 2019

View reviewed changes

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

goto-bus-stop reviewed Jun 5, 2019

View reviewed changes

arturi reviewed Jun 5, 2019

View reviewed changes

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

arturi reviewed Jun 5, 2019

View reviewed changes

packages/@uppy/provider-views/src/index.js Show resolved Hide resolved

arturi reviewed Jun 11, 2019

View reviewed changes

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

goto-bus-stop reviewed Jun 16, 2019

View reviewed changes

ifedapoolarewaju added 6 commits June 20, 2019 06:45

companion,companion-client: send uppy-versions header to companion

d77180a

companion-client: set default values to allowed headers

f626fc0

companion,companion-client: remove/add logs accordingly

3afc90b

companion,provider-views: set default version if none is set

592b43b

also encode versions sent through query params

companion-client: flatten promise usage

3d167cd

companion: use decodeURI only when value is set

cabff1b

ifedapoolarewaju force-pushed the preflight-version-header branch from 573c5ee to cabff1b Compare June 20, 2019 06:04

companion-client: add fallback for preflight failure

49539f7

goto-bus-stop approved these changes Jun 24, 2019

View reviewed changes

ifedapoolarewaju merged commit b25f243 into master Jun 26, 2019

delete-merged-branch bot deleted the preflight-version-header branch June 26, 2019 11:03

ifedapoolarewaju mentioned this pull request Jul 29, 2019

companion: set allowed http methods internally #1754

Merged

aduh95 mentioned this pull request Dec 4, 2023

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

companion,companion-client: send uppy-versions header to companion #1612

companion,companion-client: send uppy-versions header to companion #1612

ifedapoolarewaju commented May 29, 2019

kvz left a comment

kvz commented May 30, 2019 via email

goto-bus-stop left a comment

arturi commented Jun 11, 2019 •

edited

Loading

ifedapoolarewaju commented Jun 13, 2019

goto-bus-stop Jun 16, 2019 •

edited

Loading

ifedapoolarewaju Jun 18, 2019

kvz Jun 18, 2019

goto-bus-stop Jun 19, 2019

ifedapoolarewaju Jun 19, 2019

goto-bus-stop Jun 19, 2019

kvz Jun 19, 2019

ifedapoolarewaju Jun 19, 2019

ifedapoolarewaju Jun 19, 2019

ifedapoolarewaju Jun 20, 2019

goto-bus-stop left a comment

arturi commented Jun 25, 2019

ifedapoolarewaju commented Jun 26, 2019

davekiss commented Jul 25, 2019

shell362 commented Aug 18, 2019

ifedapoolarewaju commented Aug 30, 2019

shell362 commented Aug 30, 2019

ifedapoolarewaju commented Aug 30, 2019 •

edited

Loading

shell362 commented Aug 30, 2019

companion,companion-client: send uppy-versions header to companion #1612

companion,companion-client: send uppy-versions header to companion #1612

Conversation

ifedapoolarewaju commented May 29, 2019

kvz left a comment

Choose a reason for hiding this comment

kvz commented May 30, 2019 via email

goto-bus-stop left a comment

Choose a reason for hiding this comment

arturi commented Jun 11, 2019 • edited Loading

ifedapoolarewaju commented Jun 13, 2019

goto-bus-stop Jun 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goto-bus-stop left a comment

Choose a reason for hiding this comment

arturi commented Jun 25, 2019

ifedapoolarewaju commented Jun 26, 2019

davekiss commented Jul 25, 2019

shell362 commented Aug 18, 2019

ifedapoolarewaju commented Aug 30, 2019

shell362 commented Aug 30, 2019

ifedapoolarewaju commented Aug 30, 2019 • edited Loading

shell362 commented Aug 30, 2019

arturi commented Jun 11, 2019 •

edited

Loading

goto-bus-stop Jun 16, 2019 •

edited

Loading

ifedapoolarewaju commented Aug 30, 2019 •

edited

Loading