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

@uppy/companion: deprecate RequestClient options #4265

Closed
wants to merge 1 commit into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jan 5, 2023

Deprecate RequestClient options in favor of less Companion-centered ones so this class can be reused in non-Companion setup.

Deprecate `RequestClient` options in favor of less Companion-centered
ones so this class can be reused in non-Companion setup.
Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

I don't think we should make this change.

  • This seems to be an XY problem.
  • companion-client is specifically made as an interface for Companion. Therefor it's good to have that tightly coupled.
  • 99% of the use cases, which is remote file downloads, can only be done with a companion instance (standalone or integrated) and not a random backend endpoint.
  • I'm assuming the use case here is catering for S3 signing requests to your own backend which coincidentally would have the exact same structure as Companion, saving boiler plate
    • we now strongly discourage using Companion for signing, so this change seems to be exclusively for something we don't want to encourage.
    • this change would implicitly mean that changes between the Companion client and server now also impact customers, instead of just a surface area we control.
    • to me it seems more likely people would want to define their own endpoints. Making this future breaking change for an edge case usage even more edge case.
  • All the docs and examples would need to be updated (eventually) and another breaking change people would need to migrate from.

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 5, 2023

I think we should do it, not doing it forces devs to serve a ton of useless bytes re-implementing what's already in @uppy/companion-client to handle these requests.

to me it seems more likely people would want to define their own endpoints

It's part of the plan, see 686f2d0.

@Murderlon
Copy link
Member

I think we should do it, not doing it forces devs to serve a ton of useless bytes re-implementing what's already in @uppy/companion-client to handle these requests.

Did you read my points? This is a proposal for a breaking change for something that Companion should not be used for. After strong discouragement from us in the docs, if someone still likes use Companion for this, and then in the unlikely event of wanting to exactly mimic companion of this already unlikely event, I think they can just do the boiler plate. And if that's a problem, then this perhaps a XY problem after all, because we could come up with alternatives to ease that pain.

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