-
Notifications
You must be signed in to change notification settings - Fork 16
*[ft] Add Rollout Percentage functionality #81
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
base: master
Are you sure you want to change the base?
Conversation
Hello! @chiraag918 Although I haven’t received any direct requests for it yet, I believe there are definitely use cases where the progressive rollout feature will be useful. -- About the featureThe idea of using the device ID for bucketing is very interesting! I’ve been thinking about how we could add a bit of randomness to this. -- About the
|
Hello @floydkim First of all, thank you so much for taking the time to review my contribution. I really appreciate your thoughtful feedback! 🙏 Latest Commit Changes - followed your suggestion to include packageHash About the disabled flag About testing Thanks again for the valuable feedback! Please let me know if you'd like me to make any further changes. |
@chiraag918 I noticed that you added a random factor along with the package hash. I also think that adding a random factor somewhat diminishes the meaning of using the device ID and package hash. With the random factor added, doesn’t the outcome essentially become the same as just using Math.random()? 🤔 If the random factor were removed, I believe deterministic bucketing could be achieved for the same device and the same CodePush update. (If my previous comment using the phrase “a bit of randomness” caused any confusion, I sincerely apologize. 🙏) |
@floydkim true, if the user were to uninstall & reinstall the app, the previous decision can get changed depending on the random factor 😅. So in that case, keeping device ID and packageHash would solve for this use case right? I added the random factor to make it a bit more random. I can remove the random multiplier. Any other suggestions or thoughts? (No worries, these conversations helps me with understanding these corner cases, thanks 😀) |
@chiraag918 If the need ever arises, we could later add an option to make the bucketing completely random. :) |
@floydkim I've removed the random factor multiplier in my recent commit. Request you to please review the PR once. Thank you. |
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 @chiraag918 !
Sorry for the delay.
I reviewed the code thoroughly and had some questions, so I’ve asked them.
Please respond when you have time. 😄
Once I understand your answers, I’m thinking of merging the PR first and then making the changes myself, including some minor coding style tweaks.
What do you think about it?
I want to reduce the back-and-forth on feedback and move quickly toward releasing the feature.
* @param deploymentKey The deployment key to use to query the CodePush server for an update. | ||
* | ||
* @param handleBinaryVersionMismatchCallback An optional callback for handling target binary version mismatch | ||
*/ | ||
function checkForUpdate(handleBinaryVersionMismatchCallback?: HandleBinaryVersionMismatchCallback): Promise<RemotePackage | null>; | ||
function checkForUpdate(deploymentKey?: string, handleBinaryVersionMismatchCallback?: HandleBinaryVersionMismatchCallback): Promise<RemotePackage | null>; |
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.
Since deploymentKey has been deprecated, it should not be added back.
Was this change intentional?
if(remotePackage?.skipRollout){ | ||
syncStatusChangeCallback(CodePush.SyncStatus.UNKNOWN_ERROR); | ||
return CodePush.SyncStatus.UNKNOWN_ERROR; | ||
} |
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.
I think this status should be CODEPUSH_SKIPPED.
const remotePackage = { ...update, ...PackageMixins.remote() }; | ||
const remotePackage = { ...PackageMixins.remote(), ...update }; |
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.
Is it necessary to change the order of the spread operation?
// Rollout filtering | ||
const shouldApply = await shouldApplyCodePushUpdate(remotePackage, nativeConfig.clientUniqueId, sharedCodePushOptions?.onRolloutSkipped); | ||
|
||
if(!shouldApply && !remotePackage.enabled) |
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.
enabled
property is not exists.
if(prevRolloutCacheKey) | ||
await RolloutStorage.removeItem(prevRolloutCacheKey); | ||
|
||
await RolloutStorage.setItem(ROLLOUT_CACHE_KEY, rolloutKey); |
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.
It seems possible to implement without adding ROLLOUT_CACHE_KEY
..
I will take a look while refactoring the code.
} | ||
|
||
function getRolloutKey(label, rollout) { | ||
return `${ROLLOUT_CACHE_PREFIX}${label}_rollout_${rollout ?? 'full'}`; |
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.
When rollout parameter is 100, returns ${ROLLOUT_CACHE_PREFIX}${label}_rollout_${100}
,
and when it’s undefined, returns ${ROLLOUT_CACHE_PREFIX}${label}_rollout_${'full'}
.
I don’t think the two return values need to be different, and it seems fine to just use 100 directly in the key.
if(remotePackage?.skipRollout){ | ||
syncStatusChangeCallback(CodePush.SyncStatus.UNKNOWN_ERROR); | ||
return CodePush.SyncStatus.UNKNOWN_ERROR; | ||
} |
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.
Is there a particular reason why we need to distinguish the state where it’s skipped because it’s not within the rollout range?
In apps that used the UP_TO_DATE state to inform users that "the app is up to date", code changes will be necessary due to the addition of this new state.
(If not using the rollout feature, no changes would be needed, of course.)
There may be operational or management reasons, but I think it’s possible to collect that information through the onRolloutSkipped callback.
import com.facebook.react.bridge.ReactMethod; | ||
import com.facebook.react.bridge.Promise; | ||
|
||
public class RolloutStorageModule extends ReactContextBaseJavaModule { |
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.
The SettingsManager already handles interactions with SharedPreference.
Was the reason you added a new module instead to separate concerns?
@@ -16,6 +16,7 @@ program.command('release') | |||
.option('-j, --js-bundle-name <string>', 'JS bundle file name (default-ios: "main.jsbundle" / default-android: "index.android.bundle")') | |||
.option('-m, --mandatory <bool>', 'make the release to be mandatory', parseBoolean, false) | |||
.option('--enable <bool>', 'make the release to be enabled', parseBoolean, true) | |||
.option('--rollout <number>', 'rollout percentage (0-100)', parseFloat, 100) |
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.
Personally, I think it would be better not to add the rollout property when the rollout feature is not used, so that the JSON data doesn’t grow unnecessarily.
RCT_EXTERN_METHOD(setItem:(NSString *)key value:(NSString *)value) | ||
RCT_EXTERN_METHOD(getItem:(NSString *)key resolver:(RCTPromiseResolveBlock)resolve rejecter:(RCTPromiseRejectBlock)reject) | ||
RCT_EXTERN_METHOD(removeItem:(NSString *)key) |
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.
I’m not sure if the RCT_EXPORT_METHOD macro is needed in the header file as well.
From what I can find, it doesn’t seem to be commonly declared in headers.
✨ Feature: Rollout Percentage with Persistent Bucketing & Release Awareness
📌 What this PR adds
This PR introduces client-side rollout percentage support for CodePush updates, with the following features:
Rollout-based user bucketing
rollout
percentage (e.g.,30
), each device is deterministically assigned to a 0–99 bucket using a stable hash of its unique ID.Persistent rollout decisions per update
packageHash
androllout
), the decision is cached locally using AsyncStorage.Rollout re-evaluation on update change
label
or therollout
value changes, the rollout decision is recalculated and the cache is reset automatically.Respects CodePush enabled flag
enabled
flag is respected in the logic to completely bypass rollout logic if enabled is false.🧪 Behavior Summary
✅ Why this is useful
🙏 Request to Maintainers
Dear maintainers, I'd love your feedback on this PR. If this feature aligns with your goals for the package, I kindly request a review and merge.
rollout
.Thank you for maintaining this awesome package! 🙌
Let me know if you'd like me to add: