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

feat(exchanges): add binding for @urql/retry-exchange #244

Merged
merged 3 commits into from
Dec 29, 2020

Conversation

parkerziegler
Copy link
Contributor

@parkerziegler parkerziegler commented Dec 27, 2020

This PR adds support for using @urql/retry-exchange.

The binding for this is fairly thin, although we did need to introduce a makeRetryExchangeOptions function to support the creation of a retryExchangeOptions record. There are generally two mechanisms for doing this type of "object creation with potentially undefined values" workflow. The first is what we do here, where we have a function that uses optional labelled arguments to generate a record. For example, calling makeRetryExchaneOptions with no arguments (just the positional unit parameter):

open ReasonUrql

let retryExchangeOptions = Client.Exchanges.makeRetryExchangeOptions();

results in the following object on the JS-side:

{
  initialDelayMs: undefined,
  maxDelayMs: undefined,
  maxNumberAttempts: undefined,
  randomDelay: undefined,
  retryIf: undefined,
}

while calling it with some arguments:

open ReasonUrql

let retryExchangeOptions =
  Client.Exchanges.makeRetryExchangeOptions(~initialDelayMs=2000, ~randomDelay=false, ())

results in the following object on the JS-side:

{
  initialDelayMs: 2000,
  maxDelayMs: undefined,
  maxNumberAttempts: undefined,
  randomDelay: false,
  retryIf: undefined,
}

We could also use an abstract record, but these are generally recommended against because of the ergonomics. The one case where they're useful is when compiling an unspecified property explicitly to undefined would result in negative behavior. In this case, urql checks the truthiness of all properties and supplies defaults downstream, so we should be safe explicitly compiling to undefined!

Copy link

@kitten kitten left a comment

Choose a reason for hiding this comment

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

I'm a bit concerned about adding more snapshots that save the Client's shape 🤔 other than that LGTM of course ✌️

@parkerziegler
Copy link
Contributor Author

Good shout @kitten. I haven't figured out a good way of testing these new exchanges TBH. I think the tests of "can they be applied" actually aren't meaningful, because even a Client snapshot doesn't verify that the exchange is being applied. We'd need more sophisticated logic to execute requests with a Client instance and spy on some behavior of the exchange. That's more effort than I want to invest just yet, but I'll remove the unnecessary snapshots here 👍

@parkerziegler parkerziegler merged commit e72ab10 into main Dec 29, 2020
@parkerziegler parkerziegler deleted the feat/add-retry-exchange branch December 29, 2020 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants