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

Add retryExchange package #564

Merged
merged 12 commits into from
Mar 9, 2020
Merged

Add retryExchange package #564

merged 12 commits into from
Mar 9, 2020

Conversation

wgolledge
Copy link
Contributor

@wgolledge wgolledge commented Mar 3, 2020

Summary

Add a new retryExchange based on the previous implementation. This version adds some additional options.

It's an exchange factory of type Options => Exchange.

Accepted options:

  • initialDelayMs - the minimum delay between retries, defaults to 1000
  • maxDelayMs - the maximum delay that can be applied to an operation
  • randomDelay - whether to apply a random delay to protect against thundering herd, defaults to true
  • maxNumberAttempts - the maximum number of attempts to retry any given operation, defaults to Infinity
  • retryIf - optional function to apply to errors to determine whether they should be retried

Points of note

  • I've created this as an external package rather than extending core
  • Can we think of any other options that could be handy?

@changeset-bot
Copy link

changeset-bot bot commented Mar 3, 2020

💥 No Changeset

Latest commit: 64f17c8

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

exchanges/retry/package.json Outdated Show resolved Hide resolved
Will Golledge added 2 commits March 5, 2020 10:03
@wgolledge wgolledge marked this pull request as ready for review March 5, 2020 18:05
Copy link
Collaborator

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

Nice work already, two minor comments

exchanges/retry/package.json Show resolved Hide resolved
exchanges/retry/src/retryExchange.ts Outdated Show resolved Hide resolved
// Send failed responses to be retried by calling next on the retry$ subject
tap(op => nextRetryOperation(op.operation)),
// Only let through the first failed response
filter(res => !res.operation.context.retryDelay)
Copy link
Member

Choose a reason for hiding this comment

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

I’m not 100% sure this is right; so we’re letting through the first error is what the comment says rather than the last retried error in case we run out of attempts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with this, I can't see having the latest error being a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this should now be fixed

@JoviDeCroock
Copy link
Collaborator

I just realised, we should probably check for pollInterval being present so we don't retry those

"include": ["src"],
"compilerOptions": {
"baseUrl": "./",
"paths": {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is cool, but does our bundling process currently support aliasing?

I stopped using aliases after getting tired of writing alias maps in multiple places (tsconfig and webpack config)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything seems fine as far as I can see - happy to swap them to be relative paths if preferred though

@andyrichardson
Copy link
Contributor

Looks siccc 🔥

@wgolledge wgolledge requested a review from kitten March 9, 2020 11:14
Copy link
Collaborator

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

Looks awesome, just two minor comments

exchanges/retry/README.md Show resolved Hide resolved
exchanges/retry/src/retryExchange.ts Outdated Show resolved Hide resolved
@wgolledge wgolledge merged commit 8189377 into master Mar 9, 2020
@wgolledge wgolledge deleted the feat/retry-exchange branch March 9, 2020 14:17
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.

5 participants