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

move default retry logic on to defaultOptions #637

Merged
merged 4 commits into from
Oct 24, 2023

Conversation

hahn-kev
Copy link
Contributor

closes #636

it was suggested that the default value of onShouldRetry should be set to shouldRetry but that won't work because shouldRetry calls onShouldRetry, so I opted to move the code at the bottom of shouldRetry into a function declared on defaultOptions. I wasn't quite sure what we wanted to do if a user explicitly set onShouldRetry to null. Currently the default logic would still be used, so that's what I preserved here, though it does look a bit awkward.

I wasn't able to run tests or execute a build locally. I kept getting a build error about a subdirectory or file already existing.

@Acconut
Copy link
Member

Acconut commented Oct 19, 2023

On a second though, this approach is not ideal because it would be a breaking change for some users. When you are providing a custom onShouldRetry callback, you will no longer receive the check for the status code, like you used to do before.

After thinking more about this, I am unsure if we can use defaultOptions to expose shouldRetry without breaking the behavior for people supplying an onShouldRetry option.

@hahn-kev
Copy link
Contributor Author

I'm not sure what you mean. If we provide a default value and a user also provides a value the default will be overwritten, then the exact same thing that happens today does. Today if a user provides a function then the default functionality does not run. That hasn't changed.

Also, I didn't expose shouldRetry at all, I just moved some functionality inside of it into the default function. The only thing that's changed from the outside is that if you call defaultOptions.onShouldRetry you will get the default functionality if you didn't provide that option.

Copy link
Member

@Acconut Acconut left a comment

Choose a reason for hiding this comment

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

You are right, this PR does not present a breaking change for users.

I have two comments on the code. Could you also please have a look at the lint issues in upload.js: https://github.com/tus/tus-js-client/actions/runs/6573176315/job/17950213388?pr=637#step:7:43

lib/index.d.ts Show resolved Hide resolved
lib/upload.js Outdated Show resolved Hide resolved
@hahn-kev
Copy link
Contributor Author

Alright, I've pushed changes and left a comment about the type change.

When I run lint on my machine it gives a bunch of errors and warnings about line endings and files I didn't change, but I think the lint error is fixed.

Copy link
Member

@Acconut Acconut left a comment

Choose a reason for hiding this comment

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

Thank you for your help!

@Acconut Acconut merged commit 3283cb4 into tus:main Oct 24, 2023
4 of 5 checks passed
@hahn-kev hahn-kev deleted the allow-calling-default-retry branch October 24, 2023 14:50
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.

It's not possible to declare onShouldRetry and fallback to the default behaviour
2 participants