Companion: rewrite request and purest to got
#3953
Conversation
(not yet working due to jest esm issues)
Murderlon
changed the title
request and purest to gotrequest and purest to got
|
Not required for Uppy 3.0, correct? |
Not unless we want to postpone the release by two weeks (has to be tested thoroughly). It's a breaking change unfortunately. |
|
not required for companion 4, but this should be done sooner or later and will be a breaking change, so maybe companion 5 |
|
Our rough aim is no more than a major per year, so in that sense it could be either waiting two weeks or waiting 50. Yes we can deviate, but I still appreciate the reason behind this, which is to reduce friction and required attention of operating uppy on the consuming side. If 4 of us eat some discomfort so the ecosystem at large eats less of it and only has to pay attention to BREAKING notices ~once a year, that may be worth it? |
|
after downgrading |
- rewrite remaining providers to got and reuse code - port tests - remove request - remove purest - rewrite periodic ping job to got - rewrite uploader to got - rewrite "url" to got - rewrite getRedirectEvaluator/request to got - rewrite http/https agent/request to got - rewrite credentials.js to got - fix "todo: handle failures differently to return 400 for this case instead" - add test for http/https agent - improve test for credentials (remote/local) - make /zoom/logout return 424 instead of 500 on credentials error - remove useless http-agent tests - fix various eslint warnings
|
One remaining issue is that because we're running got v11, and it still depends on the old and unmaintained I have not actually manually tested:
I added some TODOs to #3284 |
| @@ -87,7 +87,7 @@ export default () => { | |||
| // .use(Unsplash, { target: Dashboard, companionUrl: COMPANION_URL, companionAllowedHosts }) | |||
| .use(RemoteSources, { | |||
| companionUrl: COMPANION_URL, | |||
| sources: ['Box', 'Dropbox', 'Facebook', 'GoogleDrive', 'Instagram', 'OneDrive', 'Unsplash', 'Url'], | |||
| sources: ['Box', 'Dropbox', 'Facebook', 'GoogleDrive', 'Instagram', 'OneDrive', 'Unsplash', 'Zoom', 'Url'], | |||
There was a problem hiding this comment.
Isn't that be causing problems if devs don't have Zoom credentials in their .env?
There was a problem hiding this comment.
Not as long as you don't try to use the Zoom plugin right?
There was a problem hiding this comment.
Yup, I use some of the others also without credentials, and it only fails once you try to use those providers
| async function withProviderErrorHandling ({ fn, tag, providerName, isAuthError, getJsonErrorMessage }) { | ||
| try { | ||
| return await fn() | ||
| } catch (err) { | ||
| const err2 = convertProviderError({ err, providerName, isAuthError, getJsonErrorMessage }) | ||
| logger.error(err2, tag) | ||
| throw err2 | ||
| } | ||
| } |
There was a problem hiding this comment.
hah! probably not a textbook pattern but I wanted to reuse the error handling logic to make it consistent across all providers. maybe it could have been done with inheritance too
There was a problem hiding this comment.
alternatively we could pull the error handling logic out of the providers and instead do it in one place in the code that calls the provider methods
There was a problem hiding this comment.
Inheritance is rarely the answer for me. I think it's alright as your basically doing inversion of control, and you're nudging towards going functional, as it's almost a Maybe monad, where you wrap a unpredictable action with flow control (stop or continue)
* rewrite to async * rewrite box and dropbox to got (not yet working due to jest esm issues) * downgrade got * update developer notes * rewrite - rewrite remaining providers to got - rewrite to async/await - pull out adapt code into adapters - provider/companion tests still todo * add zoom to dev dashboard * rewrites - rewrite remaining providers to got and reuse code - port tests - remove request - remove purest - rewrite periodic ping job to got - rewrite uploader to got - rewrite "url" to got - rewrite getRedirectEvaluator/request to got - rewrite http/https agent/request to got - rewrite credentials.js to got - fix "todo: handle failures differently to return 400 for this case instead" - add test for http/https agent - improve test for credentials (remote/local) - make /zoom/logout return 424 instead of 500 on credentials error - remove useless http-agent tests - fix various eslint warnings * work around ts error * remove forgotten change
fixes #3496
TODO
requestgetRedirectEvaluatorform-dataHttpAgent/HttpsAgentrequestandform-datafrom companion #3496Breaking changes
configpassed in its constructor object