-
Notifications
You must be signed in to change notification settings - Fork 114
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: Add retry
and retryDelay
Properties to Enhance Download Method Reliability
#734
Conversation
🦋 Changeset detectedLatest commit: 564018a The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @hexboy, thanks for another contribution!
I've reviewed the PR and left some comments there. On a side-note, I think that we could do a lot of this on JS-side only - all we need is to check if the error from native loadScript
is related to fetching data - WDYT?
packages/repack/src/modules/ScriptManager/__tests__/ScriptManager.test.ts
Outdated
Show resolved
Hide resolved
packages/repack/src/modules/ScriptManager/__tests__/ScriptManager.test.ts
Outdated
Show resolved
Hide resolved
packages/repack/android/src/main/java/com/callstack/repack/RetryInterceptor.kt
Outdated
Show resolved
Hide resolved
hey @hexboy, do you want to work on this PR or should we take this over? It's definitely something that we want to have in the next major (5.0) release. Either way your contribution is very much appreciated! |
Hey @jbroma, I'll start working on rewriting the entire PR in JavaScript. I estimate it will take one or two days to complete. Since I don't see any existing native unit tests in the project, and there may be a need to add a new native module or package, I'm not planning to write native tests for this. |
glad to hear that!
yeah, let's not worry about native tests for now 👍 |
Hey @jbroma, I just rewrote the entire PR in JavaScript. Please check it again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for rewriting this in JS! Looks good overall, few nits there and there, take a look 👇
packages/repack/src/modules/ScriptManager/NativeScriptManager.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good now 🎉 Let's fix the failing tests by not testing the protected method in the tests and instead, test loadScript
directly there since it's exposed as a public interface 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
Thank you for working on this @hexboy, much appreciated! 👏 🚀 |
Summary
This PR introduces two new properties,
retry
andretryDelay
, to theScriptConfig
within the download method. These properties aim to improve the download process by handling network failures more robustly.retry
: Specifies the number of attempts to retry the download in case of failure.retryDelay
: Defines the delay (in milliseconds) between each retry attempt.This feature aims to minimize the impact of transient network failures and improve the reliability of downloads in unstable network conditions.
Test plan