ENG-36 Feature flag pattern. See README.md for usage #2648

johnqh · 2024-12-20T21:57:37Z

See README.md for more details.

It uses an Injection, so if/when we use remote feature flags, we can change the injection in main.tsx without changing any downstream code.

linear · 2024-12-20T21:57:39Z

ENG-36 Create pattern and code for feature flags

netlify · 2024-12-20T21:58:33Z

✅ Deploy Preview for decent-interface-dev ready!

Name	Link
🔨 Latest commit	`f220107`
🔍 Latest deploy log	https://app.netlify.com/sites/decent-interface-dev/deploys/677c615d58438d0009a5de0f
😎 Deploy Preview	https://deploy-preview-2648.app.dev.decentdao.org
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

adamgall

@johnqh please see the file src/constants/common.ts, there is some additional "feature flag" code implemented there. I think can we probably safely remove all of those, they're probably all on by default at this point. Gotta double check though.

johnqh · 2025-01-06T23:07:53Z

@johnqh please see the file src/constants/common.ts, there is some additional "feature flag" code implemented there. I think can we probably safely remove all of those, they're probably all on by default at this point. Gotta double check though.

@adamgall Bridged existing functions over so no change is needed for any code using those existing flags. It is also better to extend src/constants/common.ts to add new typed flags in the future while they are using the dependency injection underneath.

DarksightKellar

This is nice. I got a couple or so change requests

DarksightKellar · 2025-01-07T10:11:14Z

src/helpers/featureFlags.ts

+  }
+
+  get(key: string): any {
+    return this.urlFlags[key] ?? import.meta.env[key];


I'd like this to instead be return import.meta.env[key] ?? this.urlFlags[key]; so that we can disable a feature on the environment level and not worry about someone setting a url param to get around it, even if it's a very unlikely scenario

This is designed purposely to have url pattern to overwrite build time feature flags. The fundamental question is whether we should allow users to access features hidden behind feature flags. For example, if we deliver code changes with feature flag enabled, and user encounters issues, we can ask him to disable the feature flag to test again, and quickly know if the issue is coded by new code. Let's discussion tomorrow

I personally think it would be good to allow both scenarios:

feature disabled at environment level, enableable at query flag level

feature enabled at environment level, disableable at query flag level

DarksightKellar · 2025-01-07T10:11:29Z

src/main.tsx

@@ -1,11 +1,12 @@
 import '@fontsource/space-mono';
 import React from 'react';
 import ReactDOM from 'react-dom/client';
-import { RouterProvider } from 'react-router-dom';
+import { RouterProvider, useSearchParams } from 'react-router-dom';


unused import useSearchParams

DarksightKellar · 2025-01-07T10:21:51Z

src/constants/common.ts

-  } else {
-    return false;
-  }
+  return FeatureFlags.instance?.get(`VITE_APP_FLAG_${feature}`) === 'ON';


Since we're bridging these two feature flag implementations, can we go all the way? I'm thinking:

Move all feature-flag code from here to helpers/featureFlags.ts (including, IMO, isDevMode on 70)

In EnvironmentFeatureFlags, use the FeatureFlag type so we're not depending on strings

Update readme to reflect that adding/removing a feature flag should also happen in const features on 58

Might as well move isFeatureEnabled into EnvironmentFeatureFlags. I dunno.

I think I can do most of these.
Let's discuss the coding style, whether to use "ON" or true/false.

Regarding coding style, i think that since the native format of Environment Variables is string, we should utilize an obvious string ("ON"), rather than pretend like it's a boolean.

I also agree with Kelvin to just consolidate both of these feature flag implementations into one.

mudrila

Love dependency injection - though I think current implementation brings mixing conceptually different approaches - and I feel like we should streamline it to either leverage existing reactivity / observability patterns with Zustand & React Router - or we should keep it "vanilla-only" and remove usage of useSearchParams, moving logic of initializing feature flags from query parameters into constructor of EnvironmentFeatureFlags

mudrila · 2025-01-07T10:14:39Z

src/main.tsx

@@ -1,11 +1,12 @@
 import '@fontsource/space-mono';
 import React from 'react';
 import ReactDOM from 'react-dom/client';
-import { RouterProvider } from 'react-router-dom';
+import { RouterProvider, useSearchParams } from 'react-router-dom';


Seems like useSearchParams is not used in this file?

mudrila · 2025-01-07T10:17:21Z

src/constants/common.ts

-  } else {
-    return false;
-  }
+  return FeatureFlags.instance?.get(`VITE_APP_FLAG_${feature}`) === 'ON';


Seems like this === 'ON' conflicts with documentation - since in the docs we're referring to the value as boolean, while here we still rely on ON.
I'd like to stick to ON value - just aesthetic matter, so loosely held preference. But yeah - we need to align implementation

mudrila · 2025-01-07T10:18:41Z

src/pages/home/HomePage.tsx

@@ -19,6 +21,11 @@ export default function HomePage() {
    }
  }, [safe?.address, action]);

+  const [searchParams] = useSearchParams();
+  searchParams.forEach((value, key) => {


We probably should wrap this forEach into useEffect to prevent re-running this loop during every re-rendering. I'm quite sure we wanna run this loop only when searchParams gets changed

I'm confused about why this code lives on the HomePage. What if a user loads a DAO directly and we bypass this HomePage component? Then I think no feature flags are going to be set

mudrila · 2025-01-07T10:22:56Z

src/helpers/featureFlags.ts

+  static instance?: IFeatureFlags;
+}
+
+export class EnvironmentFeatureFlags implements IFeatureFlags {


I'm wondering if we wanna run this with class implementation in comparison to other patterns we have in the app. I'd prefer using React context or Zustand store to gain better observability and streamline UI update when search parameters are getting changed.

Current isFeatureEnabled does not correlate with the observability and reactivity that we're gaining through useSearchParams - so we either should move logic with search params right into this class, obtaining query parameters using vanilla browser API, or refactor class into Zustand store - to keep reactivity consistent.

Dependency injection is here so we can swap .env feature flags with remote configs (Firebase or LaunchDarkly) in the future. Let me think about how to do this with existing patterns. I don't want it to observe on params because once a user overwrite the flag once, he should go through the app without the param persists in all URLs.

I agree with both of these points:

would be good to follow existing patterns, using zustand for the "in memory" persistence of a flag makes sense

being able to swap where feature flags come from is important

adamgall

John could you also implement a small "other side" implementation, so this PR goes end to end? Something small, like an emoji in the footer or something. But importantly, this should utilize your new feature flag system. I'm just curious about seeing how this works in practice.

adamgall · 2025-01-09T16:24:51Z

README.md

+
+https://app.dev.decentdao.org?FEATURE_1=true
+
+From then, the flag holds the value from the URL param until app is refreshed


So to be clear about what this means

a feature flag query param being present will set an in-memory variable

i assume that if the query param is not present, it won't unset or change a previously set in-memory value

i also assume that if the query param is present, it will set the in-memory value true/false depending on query param value

as long as accessed via FeatureFlags.instance?.get('FEATURE_1'), the in-memory value will always be returned to callers

but nothing is being persisted beyond refreshes

I am making a handful of assumptions here that i'll probably work through as i keep reading the code, but my notes here are that it seems like in-memory layer for feature flags doesn't solve much functional purpose... I'm questioning how one could get into a state where they've removed the query param but haven't triggered a browser page reload.

But this is just my rambling thoughts nothing to action on here.

edit: wait do query params not persist when you're clicking around in an app? i thought they did. maybe they don't, duh.

adamgall · 2025-01-09T17:10:41Z

src/constants/common.ts

-  } else {
-    return false;
-  }
+  return FeatureFlags.instance?.get(`VITE_APP_FLAG_${feature}`) === 'ON';


Regarding coding style, i think that since the native format of Environment Variables is string, we should utilize an obvious string ("ON"), rather than pretend like it's a boolean.

adamgall · 2025-01-09T18:14:28Z

src/constants/common.ts

-  } else {
-    return false;
-  }
+  return FeatureFlags.instance?.get(`VITE_APP_FLAG_${feature}`) === 'ON';


I also agree with Kelvin to just consolidate both of these feature flag implementations into one.

adamgall · 2025-01-09T18:19:10Z

src/helpers/featureFlags.ts

+  static instance?: IFeatureFlags;
+}
+
+export class EnvironmentFeatureFlags implements IFeatureFlags {


I agree with both of these points:

would be good to follow existing patterns, using zustand for the "in memory" persistence of a flag makes sense

being able to swap where feature flags come from is important

adamgall · 2025-01-09T19:42:33Z

src/helpers/featureFlags.ts

+  }
+
+  get(key: string): any {
+    return this.urlFlags[key] ?? import.meta.env[key];


I personally think it would be good to allow both scenarios:

feature disabled at environment level, enableable at query flag level

feature enabled at environment level, disableable at query flag level

adamgall · 2025-01-09T19:43:54Z

src/pages/home/HomePage.tsx

@@ -19,6 +21,11 @@ export default function HomePage() {
    }
  }, [safe?.address, action]);

+  const [searchParams] = useSearchParams();
+  searchParams.forEach((value, key) => {


I'm confused about why this code lives on the HomePage. What if a user loads a DAO directly and we bypass this HomePage component? Then I think no feature flags are going to be set

ENG-36 Feature flag pattern. See README.md for usage

dcda8b8

johnqh requested review from adamgall, DarksightKellar, mudrila and Da-Colon December 20, 2024 21:57

adamgall reviewed Jan 6, 2025

View reviewed changes

John Huang added 2 commits January 6, 2025 14:41

Merge branch 'develop' into issue/env-feature-flags

e7f1000

Bridge existing feature flag code over so no code changes is needed.

f220107

DarksightKellar requested changes Jan 7, 2025

View reviewed changes

mudrila requested changes Jan 7, 2025

View reviewed changes

adamgall reviewed Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENG-36 Feature flag pattern. See README.md for usage #2648

ENG-36 Feature flag pattern. See README.md for usage #2648

johnqh commented Dec 20, 2024

linear bot commented Dec 20, 2024

netlify bot commented Dec 20, 2024 •

edited

Loading

adamgall left a comment

johnqh commented Jan 6, 2025

DarksightKellar left a comment

DarksightKellar Jan 7, 2025

johnqh Jan 8, 2025

adamgall Jan 9, 2025

DarksightKellar Jan 7, 2025

DarksightKellar Jan 7, 2025

johnqh Jan 8, 2025

adamgall Jan 9, 2025

adamgall Jan 9, 2025

mudrila left a comment

mudrila Jan 7, 2025

mudrila Jan 7, 2025

mudrila Jan 7, 2025

adamgall Jan 9, 2025 •

edited

Loading

mudrila Jan 7, 2025

johnqh Jan 8, 2025

adamgall Jan 9, 2025

adamgall left a comment

adamgall Jan 9, 2025

adamgall Jan 9, 2025

adamgall Jan 9, 2025

adamgall Jan 9, 2025

adamgall Jan 9, 2025

adamgall Jan 9, 2025 •

edited

Loading


		https://app.dev.decentdao.org?FEATURE_1=true

		From then, the flag holds the value from the URL param until app is refreshed

ENG-36 Feature flag pattern. See README.md for usage #2648

Are you sure you want to change the base?

ENG-36 Feature flag pattern. See README.md for usage #2648

Conversation

johnqh commented Dec 20, 2024

linear bot commented Dec 20, 2024

netlify bot commented Dec 20, 2024 • edited Loading

✅ Deploy Preview for decent-interface-dev ready!

adamgall left a comment

Choose a reason for hiding this comment

johnqh commented Jan 6, 2025

DarksightKellar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mudrila left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamgall Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamgall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamgall Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

netlify bot commented Dec 20, 2024 •

edited

Loading

adamgall Jan 9, 2025 •

edited

Loading

adamgall Jan 9, 2025 •

edited

Loading