[Issue #3249] Feature flag manager refactor #3374

doug-s-nava · 2025-01-02T15:53:42Z

Summary

TODOS

docs

Time to review: 45 mins

Changes proposed

The existing feature flag system in NextJs consists of a withFeatureFlag wrapper that is used only on the server, a useFeatureFlag hook that is used only on the client, and a FeatureFlagManager that is by both of these implementations and within NextJs middleware. The FeatureFlagManager is able to read environment variables for feature flag values, but as currently written, is unable to make these values available to client side implementations of the FeatureFlagManager.

This change rewrites the FeatureFlagManager and useFeatureFlag hook to solve this issue, primarily making everything rely on all feature flag state being maintained in cookies to allow client and server to remain in sync.

It also:

adds the authOff feature flag in frontend code and terraform
refactors the environments setup a bit to more easily expose feature flags
splits functionality that does not benefit from being held in the FeatureFlagsManager class into a helper file
moves feature flag manager file into a nested directory
creates a placeholder "auth enabled" text in the header for testing - to be replaced with implementation from [Issue #2653] Sign in component #3281 once that is merged

Context for reviewers

For whoever is reviewing this, I'd appreciate an in person meeting to explain a few things and gather thoughts around the approach.

Test Steps

start server on this branch with npm run build && npm start
visit http://localhost:3000
VERIFY: a sign in button appears in the header
visit http://localhost:3000/opportunity/33?_ff=opportunityOff:false
VERIFY: opportunity page loads
start a server on this branch with npm run build && FEATURE_AUTH_OFF="true" FEATURE_OPPORTUNITY_OFF="true" npm start
visit http://localhost:3000
VERIFY: no sign in button appears
visit http://localhost:3000/opportunity/33?_ff=opportunityOff:false
VERIFY: opportunity page redirects to maintenance
visit http://localhost:3000?_ff=authOff:false
VERIFY: sign in button appears in the header
visit http://localhost:3000/opportunity/33?_ff=opportunityOff:false
VERIFY: opportunity page loads
visit localhost:3000/dev/feature-flags
VERIFY: both authOff and opportunityOff are disabled
turn on both authOff and opportunityOff from the UI
refresh
VERIFY: sign in button appears
visit http://localhost:3000/opportunity/33
VERIFY: you are redirected to maintenance

Additional information

With auth enabled the header should look like this

doug-s-nava · 2025-01-02T18:01:33Z

frontend/src/components/Layout.tsx


  return (
    // Stick the footer to the bottom of the page
    <div className="display-flex flex-column minh-viewport">
      <a className="usa-skipnav" href="#main-content">
        {t("Layout.skip_to_main")}
      </a>
-      <NextIntlClientProvider


this change is unrelated but at one point I was messing with this file a lot and removing this unnecessary code simplified things for me a bit

Yeah, good catch, this is already accounted for in the parent component

doug-s-nava · 2025-01-02T18:01:49Z

frontend/src/components/Header.tsx

@@ -27,6 +30,21 @@ type Props = {

 const homeRegexp = /^\/(?:e[ns])?$/;

+const LoginLink = () => {


should be removed on rebase to include actual login button

doug-s-nava · 2025-01-02T18:02:09Z

frontend/src/app/[locale]/dev/feature-flags/FeatureFlagsTable.tsx

  const { user, isLoading, error } = useUser();

-  if (!mounted) {


mounted is no longer a thing

doug-s-nava · 2025-01-02T18:03:08Z

frontend/src/constants/environments.ts

 } = process.env;

+export const featureFlags = {


this could be done elsewhere if we'd rather keep this file clean

They touch the envars so seems good to me.

doug-s-nava · 2025-01-02T18:03:57Z

frontend/src/hooks/useFeatureFlags.ts


+  // a workaround, as setting this in default state value results in hydration error
+  // on feature flag admin page. Does it cause a blip on other pages?


this seems fine but it's a workaround just for the admin page so it may not be necessary

doug-s-nava · 2025-01-02T18:05:04Z

frontend/tests/services/featureFlags/featureFlagHelpers.test.ts

+    );
+  });
+
+  // // do we still need to support this?


may need some discussion here

mdragon · 2025-01-03T21:13:29Z

frontend/src/services/featureFlags/FeatureFlagManager.ts

+    searchParams?: ServerSideSearchParams,
+  ): boolean {
+    if (!isValidFeatureFlag(name)) {
+      throw new Error(`\`${name}\` is not a valid feature flag`);


Just wondering about this down the road when we want to retire a feature flag like we did with search. I think this is throwing an error early while we're in that process and maybe missed a check because it's this throws when consuming the flag, not when trying to parse what is stuck in the user's cookies? Just trying to make sure we don't make it where we can't ever kill an old flag because some users have it set true and we will break them if it's removed from the defaults.

that's an interesting point. The way things are structured though, I'm not too worried about this causing a problem. The calls to this function should always be coming with feature flag names set internal to the app, not with names read dynamically from cookies or query params. The only issue I can see is if a feature flag was not fully removed - if we remove a flag from the list of defaults but are still attempting to use it to gate behavior, for instance.

In the case where we remove a flag but a user's cookies are still set, I think the concern is that their cookie value wouldn't be honored, but that's probably not a big issue.

In any case it's existing behavior, so we can make another ticket to look into it if you think that's warranted.

Thanks, I agree structurally we're ok, and I don't think we need to worry about not honoring them anymore once we've "dropped them" on the configurations side.

mdragon · 2025-01-03T21:42:22Z

frontend/tests/hooks/useFeatureFlags.test.ts

+
+      const { checkFeatureFlag } = result.current;
+
+      const value = checkFeatureFlag("someFakeFeature4");


Nit, but maybe making this "nonexistantFakeFeature" or "fakeFeatureDoesNotExist" as I could see 4 getting picked up by someone wanting to extend these tests further than then that would break this test. They'd realize the problem eventually, but might be easier to try to avoid the clash proactively.

mdragon · 2025-01-06T14:53:39Z

One thought, maybe for a follow up ticket. I've had issues in the past with end users getting odd stuff in their FF or even just App cookies and it can be helpful to have a URL that will just wipe out whatever they've got out there, basically just wipe clean the cookie. Often this happens on a Logout URL to other cookies, but FF ones in particular are usually not easily cleared by a non-technical user and potentially will break stuff all over the site if they're not working right. Might be worth throwing another URL arg on the flag manager or adding a _ff_clear URL argument that the middleware? would respond to by just setting an empty cookie over whatever the user had stuck in there. That way the help desk could send them a link with that special URL argument rather than as set of instructions for how to clear cookies on this site.

acouch · 2025-01-06T16:13:57Z

frontend/src/components/Layout.tsx

-import Header from "./Header";
+import Footer from "src/components/Footer";
+import GrantsIdentifier from "src/components/GrantsIdentifier";
+import Header from "src/components/Header";


We had discussed when creating #2192 that we'd allow imports that are relative if they are in the same directory. We could revisit that, but would be good to update the es lint https://github.com/HHS/simpler-grants-gov/blob/main/frontend/.eslintrc.js#L22 so we are consistent.

got it, forgot that we were allowing that. Happy to revert this

acouch · 2025-01-06T16:26:53Z

frontend/src/constants/environments.ts

-  SESSION_SECRET = "",
+  FEATURE_SEARCH_OFF,
+  FEATURE_OPPORTUNITY_OFF,
+  FEATURE_AUTH_OFF,


This feels cleaner to grab them here and set defaults in the export. Would like to alphabetize them as well, might do that in a follow-up PR.

agree alphabetizing is a good idea, also agree it might be cleaner to do it later on

acouch · 2025-01-06T16:41:47Z

One issue I'm seeing with the authOff cookie is that it flickers for users who are not using the feature flag, which would be most people visiting the site. We could fix that by defaulting to off for the auth components and renaming the flag to authOn . The login link would flicker for those using it, but that seems more acceptable as they would know about it and could be assured it is temporary (both until we go live with auth but also if we deploy the CDN first).

doug-s-nava · 2025-01-06T17:39:30Z

@mdragon if I'm understanding your idea correctly, I think we already have it implemented here:

simpler-grants-gov/frontend/src/services/FeatureFlagManager.ts

Line 194 in 965b087

paramValue === "reset"

. If you enter a query param of _ff=reset it should reset the feature flags. As far as what they reset to, currently it's defaults, and with this change it would be { ...defaults, ...flag values from env vars }

acouch · 2025-01-06T17:58:33Z

frontend/src/services/featureFlags/FeatureFlagManager.ts

+  isFeatureEnabled(
+    name: string,
+    cookies: NextRequest["cookies"] | ReadonlyRequestCookies,
+    searchParams?: ServerSideSearchParams,


These are query params, not just for the search page AFAICT, so queryParams as an arg would make more sense IMO.

not sure if you're saying that there is a better type to be using here or that we should rename the parameter. The nomenclature in the app is a bit confusing, and I confused myself enough while thinking about this that I had to look things up. https://stackoverflow.com/a/39294675 It sounds like "query params" or "search params" are not defined in any spec, but Next uses the term "search params" in their "useSearchParams" hook, and that's backed up by terminology in the URL API. "Query params" is definitely a very common phrase though. Not sure which way we'd want to go

I meant the param name. Not sure why next uses "search" but imagine it is b/c they want to make it easier for new devs to create search pages.

mdragon · 2025-01-06T18:14:10Z

@mdragon if I'm understanding your idea correctly, I think we already have it implemented here:

Yup, exactly that, I didn't notice it was already in there.

acouch

Looks great! ~~Though I would address the flicker for the login link, either in this PR or other ticket.~~

…em for client side use (#3374) * rewrites the FeatureFlagManager and useFeatureFlag hook to allow syncing flags between server and client using cookies * adds the `authOn` feature flag in frontend code and terraform * refactors the `environments` setup a bit to more easily expose feature flags * splits functionality that does not benefit from being held in the FeatureFlagsManager class into a helper file * moves feature flag manager file into a nested directory

doug-s-nava force-pushed the feature-flag-manager-refactor branch from 3438d42 to fb188d1 Compare January 2, 2025 16:35

doug-s-nava force-pushed the feature/nextjs-auth branch from ec9bc75 to 9016bbc Compare January 2, 2025 17:20

doug-s-nava force-pushed the feature-flag-manager-refactor branch from 8a26362 to 971a129 Compare January 2, 2025 17:20

doug-s-nava marked this pull request as ready for review January 2, 2025 18:00

doug-s-nava requested review from acouch, coilysiren, mdragon, andycochran and btabaska as code owners January 2, 2025 18:00

doug-s-nava commented Jan 2, 2025

View reviewed changes

doug-s-nava force-pushed the feature-flag-manager-refactor branch 2 times, most recently from 3988586 to 03f25d3 Compare January 2, 2025 20:03

doug-s-nava changed the title ~~Feature flag manager refactor~~ [Issue #3249] Feature flag manager refactor Jan 2, 2025

mdragon reviewed Jan 3, 2025

View reviewed changes

doug-s-nava added 11 commits January 3, 2025 16:21

initial attempts

e2345a2

set all feature flag values in cookie remove noCache

d87756f

i think this works

4fbe0b5

cleanup

a4ae767

adjusted feature flag manager tests, need to fix some broken ones

51f760c

tests added and all checks but e2e passing

c4eaa7c

some fix ups for e2e, hydration and accessibility

f53e715

remove unused test

914c9d4

add tf param

ad81d1e

comment cleanup, simplify login button, and add one more test

e17426a

post rebase fix

b8f15d0

doug-s-nava force-pushed the feature-flag-manager-refactor branch from 03f25d3 to b8f15d0 Compare January 3, 2025 21:28

mdragon reviewed Jan 3, 2025

View reviewed changes

acouch reviewed Jan 6, 2025

View reviewed changes

switch polarity on auth flag

721af6c

doug-s-nava added 2 commits January 6, 2025 13:51

revert relative url change

7d892ad

fix test

e68d516

acouch approved these changes Jan 6, 2025

View reviewed changes

update terraform for env var polarity switch

bde049c

doug-s-nava merged commit e358419 into feature/nextjs-auth Jan 6, 2025
14 checks passed

doug-s-nava deleted the feature-flag-manager-refactor branch January 6, 2025 20:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue #3249] Feature flag manager refactor #3374

[Issue #3249] Feature flag manager refactor #3374

doug-s-nava commented Jan 2, 2025 •

edited

Loading

doug-s-nava Jan 2, 2025

acouch Jan 6, 2025

doug-s-nava Jan 2, 2025

doug-s-nava Jan 2, 2025

doug-s-nava Jan 2, 2025

acouch Jan 6, 2025

doug-s-nava Jan 2, 2025

doug-s-nava Jan 2, 2025

mdragon Jan 3, 2025

doug-s-nava Jan 3, 2025

mdragon Jan 3, 2025

mdragon Jan 3, 2025

mdragon commented Jan 6, 2025

acouch Jan 6, 2025

doug-s-nava Jan 6, 2025

acouch Jan 6, 2025

doug-s-nava Jan 6, 2025

acouch commented Jan 6, 2025

doug-s-nava commented Jan 6, 2025

acouch Jan 6, 2025

doug-s-nava Jan 6, 2025

acouch Jan 6, 2025

mdragon commented Jan 6, 2025

acouch left a comment •

edited

Loading

		@@ -27,6 +30,21 @@ type Props = {

		const homeRegexp = /^\/(?:e[ns])?$/;

		const LoginLink = () => {

		const { user, isLoading, error } = useUser();

		if (!mounted) {


		// a workaround, as setting this in default state value results in hydration error
		// on feature flag admin page. Does it cause a blip on other pages?


		const { checkFeatureFlag } = result.current;

		const value = checkFeatureFlag("someFakeFeature4");

[Issue #3249] Feature flag manager refactor #3374

[Issue #3249] Feature flag manager refactor #3374

Conversation

doug-s-nava commented Jan 2, 2025 • edited Loading

Summary

TODOS

Time to review: 45 mins

Changes proposed

Context for reviewers

Test Steps

Additional information

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdragon commented Jan 6, 2025

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

acouch commented Jan 6, 2025

doug-s-nava commented Jan 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdragon commented Jan 6, 2025

acouch left a comment • edited Loading

Choose a reason for hiding this comment

doug-s-nava commented Jan 2, 2025 •

edited

Loading

acouch left a comment •

edited

Loading