@uppy/companion: merge Provider/SearchProvider #4330

mifi · 2023-02-23T06:52:24Z

closes #4308

NOTE: breaking change!

EDIT: not breaking after all, see #4330 (comment)

closes #4308

Murderlon

Very nice, this was exactly what I was hoping for, to use different middlewares to split the concerns :)

Murderlon · 2023-02-27T09:55:35Z

packages/@uppy/companion/src/server/provider/Provider.js

+module.exports.noAuthProvider = noAuthProvider
+
+// OAuth providers are those that have a `static authProvider` set. It means they require OAuth authentication to work
+module.exports.isOAuthProvider = (authProvider) => authProvider !== noAuthProvider


Do we need this const noAuthProvider as an empty string? It's not very intention revealing to me that we use an empty string. Can we use null instead?

Murderlon · 2023-02-27T09:58:03Z

packages/@uppy/companion/src/server/provider/index.js

-      callback: `/${providerName}/callback`,
-      transport: 'session',
+
+    if (isOAuthProvider(customProvider.module.authProvider)) {


Where is addCustomProviders used? I liked that you separated most things in different middlewares, but here we use if statements in existing logic. Are you aware of some alternatives or does this make the most sense?

the function is being used here:
https://github.com/transloadit/uppy/blob/main/packages/@uppy/companion/src/companion.js#L79

but here we use if statements in existing logic. Are you aware of some alternatives or does this make the most sense?
not sure what you mean by this

mifi

I agree we should just rewrite it to null!

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

packages/@uppy/companion/src/server/provider/Provider.js

mifi · 2023-03-04T10:16:52Z

packages/@uppy/companion/src/server/provider/index.js

-      callback: `/${providerName}/callback`,
-      transport: 'session',
+
+    if (isOAuthProvider(customProvider.module.authProvider)) {


the function is being used here:
https://github.com/transloadit/uppy/blob/main/packages/@uppy/companion/src/companion.js#L79

but here we use if statements in existing logic. Are you aware of some alternatives or does this make the most sense?
not sure what you mean by this

need to have an authProvider for a grantConfig to be set previously we didn't care, so this test was a bit wrong

Murderlon · 2023-03-13T10:08:34Z

packages/@uppy/companion/src/server/provider/Provider.js

   * @returns {string}
   */
  static get authProvider () {
-    return ''
+    return undefined


If we make this null we can make the check below more strict I think

Suggested change

return undefined

return null

I think we don't need this then?

packages/@uppy/companion/src/server/provider/Provider.js

Murderlon · 2023-03-13T12:19:36Z

Since this is a breaking change. Are we ready to merge this and work towards a major release? If not, better hold off on merging for now.

mifi · 2023-03-13T13:32:04Z

yes, I wanted to combine this with other breaking changes like #4317 - but if we're not doing the got upgrade, then I think we can do a release with just this pr and the jsonwebtoken pr #4258

Murderlon

Approved if you feel this is tested well enough. If not, we should discuss some extra cases we can test?

mifi · 2023-03-19T04:51:43Z

Cool! Well all our tests went through, and I cannot think of any particular things right now. But if something were to break, we should write a regression test for sure!

mifi · 2023-03-28T06:22:50Z

Actually when I think about it there are no breaking changes, unless you implemented a custom SearchProvider. But custom SearchProviders were not possible to implement in the first place (we only had Unsplash), so AFAIK there are no breaking changes.

| Package | Version | Package | Version | | ---------------------- | ------- | ---------------------- | ------- | | @uppy/aws-s3 | 3.0.6 | @uppy/status-bar | 3.1.0 | | @uppy/aws-s3-multipart | 3.1.3 | @uppy/transloadit | 3.1.2 | | @uppy/companion | 4.4.0 | @uppy/tus | 3.0.6 | | @uppy/companion-client | 3.1.2 | @uppy/unsplash | 3.2.0 | | @uppy/core | 3.1.2 | @uppy/url | 3.3.0 | | @uppy/dashboard | 3.3.2 | @uppy/utils | 5.2.0 | | @uppy/locales | 3.1.0 | @uppy/xhr-upload | 3.1.1 | | @uppy/provider-views | 3.2.0 | uppy | 3.7.0 | | @uppy/react | 3.1.1 | | | - @uppy/aws-s3-multipart,@uppy/aws-s3,@uppy/tus,@uppy/xhr-upload: make sure that we reset serverToken when an upload fails (Mikael Finstad / #4376) - @uppy/aws-s3-multipart: do not auto-open sockets, clean them up on abort (Antoine du Hamel) - @uppy/aws-s3: Update types (Minh Hieu / #4294) - @uppy/companion-client: do not open socket more than once (Artur Paikin) - @uppy/companion: add `service: 'companion'` to periodic ping (Mikael Finstad / #4383) - @uppy/companion: add connection keep-alive to dropbox (Mikael Finstad / #4365) - @uppy/companion: add missing env variable for standalone option (Mikael Finstad / #4382) - @uppy/companion: add S3 prefix env variable (Mikael Finstad / #4320) - @uppy/companion: allow local ips when testing (Mikael Finstad / #4328) - @uppy/companion: fix typo in redis-emitter.js (Ikko Eltociear Ashimine / #4362) - @uppy/companion: merge Provider/SearchProvider (Mikael Finstad / #4330) - @uppy/companion: only body parse when needed & increased body size for s3 (Mikael Finstad / #4372) - @uppy/core: fix bug with `setOptions` (Nguyễn bảo Trung / #4350) - @uppy/locales: locales: add es_MX (Kevin van Zonneveld / #4393) - @uppy/locales: locales: add hi_IN (Kevin van Zonneveld / #4391) - @uppy/provider-views: fix race condition when adding folders (Mikael Finstad / #4384) - @uppy/provider-views: UI: Use form attribite with a form in doc root to prevent outer form submit (Artur Paikin / #4283) - @uppy/transloadit: fix socket error message (Artur Paikin / #4352) - @uppy/tus: do not auto-open sockets, clean them up on abort (Antoine du Hamel) - meta: add version info in the bundlers CI (Antoine du Hamel / #4386) - meta: deploy to Heroku on every companion commit (Mikael Finstad / #4367) - meta: example: migrate `redux` to ESM (Antoine du Hamel / #4158) - meta: fix all ESLint warnings and turn them into errors (Antoine du Hamel / #4398) - meta: fixup! website: update links to work under the new URL (Antoine du Hamel / #4371) - meta: remove duplicate outdated OSS support docs (Mikael Finstad, Artur Paikin / #4364) - meta: use overrides to make sure no uppy package is fetch from npm (Antoine du Hamel / #4395) - website: add a deprecation notice and a link to the new website (Antoine du Hamel / #4370) - website: fix home page (Antoine du Hamel) - website: Remove the website (Merlijn Vos / #4369) - website: update links to work under the new URL (Antoine du Hamel / #4371)

* merge search provider and provider closes transloadit#4308 * simplify and improve naming * Apply suggestions from code review * fix test need to have an authProvider for a grantConfig to be set previously we didn't care, so this test was a bit wrong * Update packages/@uppy/companion/src/server/provider/Provider.js * fix merge oops

| Package | Version | Package | Version | | ---------------------- | ------- | ---------------------- | ------- | | @uppy/aws-s3 | 3.0.6 | @uppy/status-bar | 3.1.0 | | @uppy/aws-s3-multipart | 3.1.3 | @uppy/transloadit | 3.1.2 | | @uppy/companion | 4.4.0 | @uppy/tus | 3.0.6 | | @uppy/companion-client | 3.1.2 | @uppy/unsplash | 3.2.0 | | @uppy/core | 3.1.2 | @uppy/url | 3.3.0 | | @uppy/dashboard | 3.3.2 | @uppy/utils | 5.2.0 | | @uppy/locales | 3.1.0 | @uppy/xhr-upload | 3.1.1 | | @uppy/provider-views | 3.2.0 | uppy | 3.7.0 | | @uppy/react | 3.1.1 | | | - @uppy/aws-s3-multipart,@uppy/aws-s3,@uppy/tus,@uppy/xhr-upload: make sure that we reset serverToken when an upload fails (Mikael Finstad / transloadit#4376) - @uppy/aws-s3-multipart: do not auto-open sockets, clean them up on abort (Antoine du Hamel) - @uppy/aws-s3: Update types (Minh Hieu / transloadit#4294) - @uppy/companion-client: do not open socket more than once (Artur Paikin) - @uppy/companion: add `service: 'companion'` to periodic ping (Mikael Finstad / transloadit#4383) - @uppy/companion: add connection keep-alive to dropbox (Mikael Finstad / transloadit#4365) - @uppy/companion: add missing env variable for standalone option (Mikael Finstad / transloadit#4382) - @uppy/companion: add S3 prefix env variable (Mikael Finstad / transloadit#4320) - @uppy/companion: allow local ips when testing (Mikael Finstad / transloadit#4328) - @uppy/companion: fix typo in redis-emitter.js (Ikko Eltociear Ashimine / transloadit#4362) - @uppy/companion: merge Provider/SearchProvider (Mikael Finstad / transloadit#4330) - @uppy/companion: only body parse when needed & increased body size for s3 (Mikael Finstad / transloadit#4372) - @uppy/core: fix bug with `setOptions` (Nguyễn bảo Trung / transloadit#4350) - @uppy/locales: locales: add es_MX (Kevin van Zonneveld / transloadit#4393) - @uppy/locales: locales: add hi_IN (Kevin van Zonneveld / transloadit#4391) - @uppy/provider-views: fix race condition when adding folders (Mikael Finstad / transloadit#4384) - @uppy/provider-views: UI: Use form attribite with a form in doc root to prevent outer form submit (Artur Paikin / transloadit#4283) - @uppy/transloadit: fix socket error message (Artur Paikin / transloadit#4352) - @uppy/tus: do not auto-open sockets, clean them up on abort (Antoine du Hamel) - meta: add version info in the bundlers CI (Antoine du Hamel / transloadit#4386) - meta: deploy to Heroku on every companion commit (Mikael Finstad / transloadit#4367) - meta: example: migrate `redux` to ESM (Antoine du Hamel / transloadit#4158) - meta: fix all ESLint warnings and turn them into errors (Antoine du Hamel / transloadit#4398) - meta: fixup! website: update links to work under the new URL (Antoine du Hamel / transloadit#4371) - meta: remove duplicate outdated OSS support docs (Mikael Finstad, Artur Paikin / transloadit#4364) - meta: use overrides to make sure no uppy package is fetch from npm (Antoine du Hamel / transloadit#4395) - website: add a deprecation notice and a link to the new website (Antoine du Hamel / transloadit#4370) - website: fix home page (Antoine du Hamel) - website: Remove the website (Merlijn Vos / transloadit#4369) - website: update links to work under the new URL (Antoine du Hamel / transloadit#4371)

mifi added 2 commits February 23, 2023 11:32

merge search provider and provider

36d2dfd

closes #4308

simplify and improve naming

51d0b26

Murderlon reviewed Feb 27, 2023

View reviewed changes

Murderlon changed the title ~~Merge Provider/SearchProvider~~ @uppy/companion: merge Provider/SearchProvider Feb 27, 2023

mifi commented Mar 4, 2023

View reviewed changes

mifi requested a review from Murderlon March 13, 2023 09:29

mifi added 2 commits March 13, 2023 17:30

Apply suggestions from code review

7753c68

fix test

a799191

need to have an authProvider for a grantConfig to be set previously we didn't care, so this test was a bit wrong

Murderlon reviewed Mar 13, 2023

View reviewed changes

mifi commented Mar 13, 2023

View reviewed changes

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

Update packages/@uppy/companion/src/server/provider/Provider.js

6461631

kvz assigned mifi Mar 13, 2023

kvz added the semver-major label Mar 13, 2023

Murderlon reviewed Mar 14, 2023

View reviewed changes

Murderlon approved these changes Mar 20, 2023

View reviewed changes

mifi added 2 commits March 28, 2023 15:10

Merge branch 'main' into merge-provider-search-provider

d3f7c7d

fix merge oops

4306486

mifi merged commit 596989d into main Mar 28, 2023

mifi deleted the merge-provider-search-provider branch March 28, 2023 06:24

mifi removed the semver-major label Mar 28, 2023

github-actions bot mentioned this pull request Apr 4, 2023

Release: uppy@3.7.0 #4400

Merged

StrixOSG mentioned this pull request May 17, 2023

Custom providers no longer work #4460

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

@uppy/companion: merge Provider/SearchProvider #4330

@uppy/companion: merge Provider/SearchProvider #4330

mifi commented Feb 23, 2023 •

edited

Murderlon left a comment

Murderlon Feb 27, 2023

Murderlon Feb 27, 2023

mifi Mar 4, 2023

mifi left a comment

mifi Mar 4, 2023

Murderlon Mar 13, 2023

mifi Mar 13, 2023

Murderlon commented Mar 13, 2023

mifi commented Mar 13, 2023 •

edited

Murderlon left a comment

mifi commented Mar 19, 2023

mifi commented Mar 28, 2023

@uppy/companion: merge Provider/SearchProvider #4330

@uppy/companion: merge Provider/SearchProvider #4330

Conversation

mifi commented Feb 23, 2023 • edited

Murderlon left a comment

Choose a reason for hiding this comment

Murderlon Feb 27, 2023

Choose a reason for hiding this comment

Murderlon Feb 27, 2023

Choose a reason for hiding this comment

mifi Mar 4, 2023

Choose a reason for hiding this comment

mifi left a comment

Choose a reason for hiding this comment

mifi Mar 4, 2023

Choose a reason for hiding this comment

Murderlon Mar 13, 2023

Choose a reason for hiding this comment

mifi Mar 13, 2023

Choose a reason for hiding this comment

Murderlon commented Mar 13, 2023

mifi commented Mar 13, 2023 • edited

Murderlon left a comment

Choose a reason for hiding this comment

mifi commented Mar 19, 2023

mifi commented Mar 28, 2023

mifi commented Feb 23, 2023 •

edited

mifi commented Mar 13, 2023 •

edited