-
Notifications
You must be signed in to change notification settings - Fork 16
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
[Issue #3249] Feature flag manager refactor #3374
Changes from 11 commits
e2345a2
d87756f
4fbe0b5
a4ae767
51f760c
c4eaa7c
f53e715
914c9d4
ad81d1e
e17426a
b8f15d0
721af6c
7d892ad
e68d516
bde049c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,9 @@ | ||
import pick from "lodash/pick"; | ||
|
||
import { | ||
NextIntlClientProvider, | ||
useMessages, | ||
useTranslations, | ||
} from "next-intl"; | ||
import { useTranslations } from "next-intl"; | ||
import { setRequestLocale } from "next-intl/server"; | ||
|
||
import Footer from "./Footer"; | ||
import GrantsIdentifier from "./GrantsIdentifier"; | ||
import Header from "./Header"; | ||
import Footer from "src/components/Footer"; | ||
import GrantsIdentifier from "src/components/GrantsIdentifier"; | ||
import Header from "src/components/Header"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. got it, forgot that we were allowing that. Happy to revert this |
||
|
||
type Props = { | ||
children: React.ReactNode; | ||
|
@@ -20,20 +14,14 @@ export default function Layout({ children, locale }: Props) { | |
setRequestLocale(locale); | ||
|
||
const t = useTranslations(); | ||
const messages = useMessages(); | ||
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, good catch, this is already accounted for in the parent component |
||
locale={locale} | ||
messages={pick(messages, "Header")} | ||
> | ||
<Header locale={locale} /> | ||
</NextIntlClientProvider> | ||
<Header locale={locale} /> | ||
<main id="main-content">{children}</main> | ||
<Footer /> | ||
<GrantsIdentifier /> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,9 @@ | ||
import { FeatureFlags } from "src/services/FeatureFlagManager"; | ||
export type FeatureFlags = { [name: string]: boolean }; | ||
|
||
// Feature flags should default to false | ||
export const featureFlags: FeatureFlags = { | ||
export const defaultFeatureFlags: FeatureFlags = { | ||
// Kill switches for search and opportunity pages, will show maintenance page when turned on | ||
searchOff: false, | ||
opportunityOff: false, | ||
authOff: false, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,38 +1,45 @@ | ||
import { stringToBoolean } from "src/utils/generalUtils"; | ||
|
||
const { | ||
NODE_ENV, | ||
NEXT_PUBLIC_BASE_PATH, | ||
USE_SEARCH_MOCK_DATA = "", | ||
USE_SEARCH_MOCK_DATA, | ||
SENDY_API_URL, | ||
SENDY_API_KEY, | ||
SENDY_LIST_ID, | ||
API_URL, | ||
API_AUTH_TOKEN = "", | ||
API_AUTH_TOKEN, | ||
NEXT_BUILD, | ||
SESSION_SECRET, | ||
NEXT_PUBLIC_BASE_URL, | ||
FEATURE_SEARCH_OFF = "false", | ||
FEATURE_OPPORTUNITY_OFF = "false", | ||
NEXT_BUILD = "false", | ||
SESSION_SECRET = "", | ||
FEATURE_SEARCH_OFF, | ||
FEATURE_OPPORTUNITY_OFF, | ||
FEATURE_AUTH_OFF, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agree alphabetizing is a good idea, also agree it might be cleaner to do it later on |
||
AUTH_LOGIN_URL, | ||
} = process.env; | ||
|
||
export const featureFlags = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this could be done elsewhere if we'd rather keep this file clean There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They touch the envars so seems good to me. |
||
opportunityOff: stringToBoolean(FEATURE_OPPORTUNITY_OFF), | ||
searchOff: stringToBoolean(FEATURE_SEARCH_OFF), | ||
authOff: stringToBoolean(FEATURE_AUTH_OFF), | ||
}; | ||
|
||
// home for all interpreted server side environment variables | ||
export const environment: { [key: string]: string } = { | ||
LEGACY_HOST: | ||
NODE_ENV === "production" | ||
? "https://grants.gov" | ||
: "https://test.grants.gov", | ||
NEXT_PUBLIC_BASE_PATH: NEXT_PUBLIC_BASE_PATH ?? "", | ||
USE_SEARCH_MOCK_DATA, | ||
USE_SEARCH_MOCK_DATA: USE_SEARCH_MOCK_DATA || "", | ||
SENDY_API_URL: SENDY_API_URL || "", | ||
SENDY_API_KEY: SENDY_API_KEY || "", | ||
SENDY_LIST_ID: SENDY_LIST_ID || "", | ||
API_URL: API_URL || "", | ||
API_AUTH_TOKEN, | ||
AUTH_LOGIN_URL: AUTH_LOGIN_URL || "", | ||
NEXT_PUBLIC_BASE_URL: NEXT_PUBLIC_BASE_URL || "http://localhost:3000", | ||
API_AUTH_TOKEN: API_AUTH_TOKEN || "", | ||
GOOGLE_TAG_MANAGER_ID: "GTM-MV57HMHS", | ||
FEATURE_OPPORTUNITY_OFF, | ||
FEATURE_SEARCH_OFF, | ||
NEXT_BUILD, | ||
SESSION_SECRET, | ||
NEXT_BUILD: NEXT_BUILD || "false", | ||
SESSION_SECRET: SESSION_SECRET || "", | ||
NEXT_PUBLIC_BASE_URL: NEXT_PUBLIC_BASE_URL || "http://localhost:3000", | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,48 +1,71 @@ | ||
"use client"; | ||
|
||
import Cookies from "js-cookie"; | ||
import { FeatureFlagsManager } from "src/services/FeatureFlagManager"; | ||
import { isBoolean } from "lodash"; | ||
import { | ||
defaultFeatureFlags, | ||
FeatureFlags, | ||
} from "src/constants/defaultFeatureFlags"; | ||
import { | ||
FEATURE_FLAGS_KEY, | ||
getCookieExpiration, | ||
} from "src/services/featureFlags/featureFlagHelpers"; | ||
|
||
import { useEffect, useState } from "react"; | ||
import { useCallback, useEffect, useState } from "react"; | ||
|
||
/** | ||
* React hook for reading and managing feature flags in client-side code. | ||
* | ||
* ``` | ||
* function MyComponent() { | ||
* const { | ||
* featureFlagsManager, // An instance of FeatureFlagsManager | ||
* mounted, // Useful for hydration | ||
* setFeatureFlag, // Proxy for featureFlagsManager.setFeatureFlagCookie that handles updating state | ||
* } = useFeatureFlags() | ||
* | ||
* if (featureFlagsManager.isFeatureEnabled("someFeatureFlag")) { | ||
* // Do something | ||
* } | ||
* | ||
* if (!mounted) { | ||
* // To allow hydration | ||
* return null | ||
* } | ||
* Allows client components to access feature flags by | ||
* - setting the cookie | ||
* - reading the cookie | ||
* | ||
* return ( | ||
* ... | ||
* ) | ||
* } | ||
* ``` | ||
*/ | ||
export function useFeatureFlags() { | ||
const [featureFlagsManager, setFeatureFlagsManager] = useState( | ||
new FeatureFlagsManager(Cookies), | ||
); | ||
const [mounted, setMounted] = useState(false); | ||
export function useFeatureFlags(): { | ||
setFeatureFlag: (flagName: string, value: boolean) => void; | ||
checkFeatureFlag: (flagName: string) => boolean; | ||
featureFlags: FeatureFlags; | ||
} { | ||
const [featureFlags, setFeatureFlags] = | ||
useState<FeatureFlags>(defaultFeatureFlags); | ||
|
||
// 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? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this seems fine but it's a workaround just for the admin page so it may not be necessary |
||
useEffect(() => { | ||
setMounted(true); | ||
const flagsFromCookie = JSON.parse( | ||
Cookies.get(FEATURE_FLAGS_KEY) || "{}", | ||
) as FeatureFlags; | ||
setFeatureFlags(flagsFromCookie); | ||
}, []); | ||
|
||
function setFeatureFlag(name: string, value: boolean) { | ||
featureFlagsManager.setFeatureFlagCookie(name, value); | ||
setFeatureFlagsManager(new FeatureFlagsManager(Cookies)); | ||
} | ||
// Note that values set in cookies will be persistent per browser session unless explicitly overwritten | ||
const setFeatureFlag = useCallback( | ||
(flagName: string, value: boolean) => { | ||
const newFlags = { | ||
...featureFlags, | ||
[flagName]: value, | ||
}; | ||
setFeatureFlags(newFlags); | ||
Cookies.set(FEATURE_FLAGS_KEY, JSON.stringify(newFlags), { | ||
expires: getCookieExpiration(), | ||
}); | ||
}, | ||
[featureFlags, setFeatureFlags], | ||
); | ||
|
||
const checkFeatureFlag = useCallback( | ||
(flagName: string): boolean => { | ||
const value = featureFlags[flagName]; | ||
if (!isBoolean(value)) { | ||
console.error("Unknown or misconfigured feature flag: ", flagName); | ||
return false; | ||
} | ||
return value; | ||
}, | ||
[featureFlags], | ||
); | ||
|
||
return { featureFlagsManager, mounted, setFeatureFlag }; | ||
return { | ||
setFeatureFlag, | ||
checkFeatureFlag, | ||
featureFlags, | ||
}; | ||
} |
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.
mounted
is no longer a thing