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

Undo breaking change to selectHttpOptionsBody signature #9103

Merged

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Nov 22, 2021

As pointed out by @SimenB in #8699 (comment), adding an additional positional argument (printer: Printer) to the publicly-exported selectHttpOptionsBody helper function was a breaking change for direct consumers of that function. This PR restores selectHttpOptionsBody to its previous signature (defaulting to defaultPrinter), using the new selectHttpOptionsBodyInternal function for the bulk of the implementation.

Comment on lines -114 to -119
export const selectHttpOptionsAndBody = (
export function selectHttpOptionsAndBody(
operation: Operation,
printer: Printer,
fallbackConfig: HttpConfig,
...configs: Array<HttpConfig>
) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't find a way to make the printer parameter optional without creating a new function, since selectHttpOptionsAndBody already takes a ...configs rest parameter and thus can't have other optional positional parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

maaaaybe a

function selectHttpOptionsAndBody(
  operation: Operation,
  printerOrFallbackConfig: Printer | HttpConfig,
  ...configs: Array<HttpConfig>
)

then a typeof function to determine type and then either add to configs and default printer, or use provided printer?

haven't tried it out, but it at least seems like something that should work (overloading sorta sucks, but meh. can be cleaned up in a major, and keeps both backwards compat to 3.4 and 3.5)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, that would work (thanks!)… but on balance I think the two-function solution is a bit simpler.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants