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

feature flag framework #308

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from
Open

feature flag framework #308

wants to merge 9 commits into from

Conversation

HYDRO2070
Copy link
Contributor

PR Description:
1.Created API endpoints to fetch all features or a specific feature.
2.Implemented a Feature Flag Context using React Context API to manage feature states globally.
3.Added a provider to wrap the app and enable access to feature flags across components.

For example, it is used for the profile description. Similarly, we can use it across different pages.

Copy link

vercel bot commented Feb 15, 2025

@HYDRO2070 is attempting to deploy a commit to the thieflord06's projects Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

vercel bot commented Feb 15, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
clearsky-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 9, 2025 11:04pm

Copy link
Collaborator

@noahm noahm left a comment

Choose a reason for hiding this comment

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

I pushed some minor stylistic updates myself, but this looked great already! Thanks for taking care of this!

@HYDRO2070
Copy link
Contributor Author

@noahm @thieflord06
This PR implements a device-based rollout percentage mechanism for feature flags. Instead of relying on the backend for rollout logic, the client now determines whether a feature is enabled using a randomly generated device ID stored in localStorage.

@thieflord06
Copy link
Member

Looks like the functionality is working with the rollout 🔥

@thieflord06 thieflord06 requested a review from noahm February 22, 2025 03:45
@thieflord06
Copy link
Member

thieflord06 commented Feb 22, 2025

  • framework
  • identify all features
  • Backend determination logic
  • frontend determination rollout logic
  • apply to all features

Copy link
Collaborator

@noahm noahm left a comment

Choose a reason for hiding this comment

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

I think the ideal api that's not quite present here yet is a useFeatureFlag hook that accepts a string feature flag name, looks up the device id, hashes a concatenation of the id and feature name so each gets its own separate "roll". Ultimately it should just return a boolean for whether the feature is enabled.

That should be easy to accomplish with all the pieces that are already here, and it will clean up the render logic in the individual feature components greatly.

@thieflord06
Copy link
Member

If I understand correctly that's already in place, if you use the same API and pass the {feature} it returns the status.

Returns all features available:
/api/v1/anon/features

Returns if a specific feature is available:
/api/v1/anon/features/{feature_name}

@thieflord06
Copy link
Member

I didn't understand lol

I think this referring to the frontend.

@noahm
Copy link
Collaborator

noahm commented Feb 23, 2025

Right we do have a hook by that name in this pr, but it only calls out to the feature specific backend endpoint. It hasn't been updated since we switched to a client side rollout design.

@HYDRO2070
Copy link
Contributor Author

HYDRO2070 commented Mar 2, 2025

@noahm
changed the complex code in the useDeviceID hook.

@HYDRO2070
Copy link
Contributor Author

HYDRO2070 commented Mar 2, 2025

I think the ideal api that's not quite present here yet is a useFeatureFlag hook that accepts a string feature flag name, looks up the device id, hashes a concatenation of the id and feature name so each gets its own separate "roll". Ultimately it should just return a boolean for whether the feature is enabled.

That should be easy to accomplish with all the pieces that are already here, and it will clean up the render logic in the individual feature components greatly.

@noahm
Are you suggesting that all the processing should be done inside the hook, and it should only return the feature value, completely changing the existing logic?

@noahm
Copy link
Collaborator

noahm commented Mar 3, 2025

@noahm Are you suggesting that all the processing should be done inside the hook, and it should only return the feature value, completely changing the existing logic?

Yes, processing should be done inside hooks to keep the render logic in actual components cleaner.

Won't it be easier to just store the rollout number so we don't have to calculate it every time? If not, then I will change it.

You're right, it is a bit wasteful to have to re-calculate every time, but that's just a matter of adding a caching layer of some kind. We can take care of that as a final step after everything else is in place and working with a clean API shape.

@HYDRO2070
Copy link
Contributor Author

HYDRO2070 commented Mar 4, 2025

@noahm Are you suggesting that all the processing should be done inside the hook, and it should only return the feature value, completely changing the existing logic?

Yes, processing should be done inside hooks to keep the render logic in actual components cleaner.

Won't it be easier to just store the rollout number so we don't have to calculate it every time? If not, then I will change it.

You're right, it is a bit wasteful to have to re-calculate every time, but that's just a matter of adding a caching layer of some kind. We can take care of that as a final step after everything else is in place and working with a clean API shape.

Firstly can you please describe like how it's working will be inside the hook?
Will there be two hook. First one for fetching all the values and second for taking rollout percentage, comparing and returning bool value. Is this right?

And for second one for storing rollout in local. How would you suggest to handle that. Right now?

@HYDRO2070 HYDRO2070 requested a review from noahm March 4, 2025 11:12
@noahm
Copy link
Collaborator

noahm commented Mar 5, 2025

Firstly can you please describe like how it's working will be inside the hook? Will there be two hook. First one for fetching all the values and second for taking rollout percentage, comparing and returning bool value. Is this right?

Yes, there will be too hooks. One for fetching which we already have finished (this is useAllFeatures), and another that we already have all the pieces for in this PR, but which are assembled in an awkward way and need to be shuffled around a bit. (a useFeatureFlag(flagName: string): boolean hook that I'm proposing)

to pseudo-code that hook a bit:

/**
 * returns null if backend is still fetching or returned an error,
 * otherwise, returns true if given flag is enabled, or false if disabled
 */
function useFeatureFlag(flagName: string): boolean | null {
  const {isLoading, isErrored, data: backendData } = useAllFeatures();

  // all remaining work in this hook is only re-calculated if backend data changes
  return useMemo(() => {
    if (isLoading || isErrored) return null;
    const selectedFlag = backendData[flagName];

    // there is no rollout specified, so just return the baseline status
    if (!selectedFlag || !selectedFlag.rollout) return selectedFlag?.status || false;

    // there is a rollout specified, so we need to use our deviceId to determine enrollment
    const deviceId = getOrCreateDeviceId();

    // note the hash shouldn't be based on deviceId alone, but also on this specific flag.
    const percentage = hashStringToPercentage(deviceId + ':' + flagName);

    // return the final boolean on whether this flag is enabled or not for this device
    return percentage < selectedFlag.rollout;
  }, [isLoading, isErrored, backendData]);
}

And for second one for storing rollout in local. How would you suggest to handle that. Right now?

No, we never want to store the calculated rollout in storage. Only the device id gets saved to localstorage.

@HYDRO2070
Copy link
Contributor Author

a useFeatureFlag(flagName: string): boolean hook that I'm proposing)

@noahm
I have moved the checking and comparison logic inside the hook. Now, it only stores the deviceId as a string. I also used useMemo to optimize the retrieval of deviceId (if it works as intended). Additionally, I hash the deviceId along with the flagName for better distribution.

Copy link
Collaborator

@noahm noahm left a comment

Choose a reason for hiding this comment

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

everything looks really good here now! just a couple of very minor things that need to be addressed now.

@@ -34,31 +35,37 @@ export function HomeStatsMain({

{loading ? undefined : (
<Button size="small" className="toggle-table" onClick={onToggleTable}>
<ViewList style={{ color: 'gray' }} />
{useFeatureFlag('stats-page') && <ViewList style={{ color: 'gray' }} />}
Copy link
Collaborator

Choose a reason for hiding this comment

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

because this is used underneath the loading ? check just above, this is conditionally calling a hook which is against the rules. This needs to be hoisted up and have the return value saved as a variable to reference here.

Comment on lines 54 to +63
{stats && (
<>
<TopBlocked
blocked={stats.topLists.total.blocked}
blocked24={stats.topLists['24h'].blocked}
/>
{useFeatureFlag('top-blocked') && (
<TopBlocked
blocked={stats.topLists.total.blocked}
blocked24={stats.topLists['24h'].blocked}
/>
)}

<TopBlockers
blockers={stats.topLists.total.blockers}
blockers24={stats.topLists['24h'].blockers}
/>
{useFeatureFlag('top-blockers') && (
Copy link
Collaborator

@noahm noahm Mar 9, 2025

Choose a reason for hiding this comment

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

same for these two. they're also conditional (per the stats && check) and need to be hoisted up.

@@ -22,7 +21,9 @@ export function HomeStatsMain({
loading,
stats,
onToggleTable,
features,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is unused and can be removed

Copy link
Collaborator

Choose a reason for hiding this comment

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

all changes in this file can be reverted now that we have the more unified feature flag hook.

@thieflord06 thieflord06 linked an issue Mar 10, 2025 that may be closed by this pull request
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.

feature flag framework
3 participants