-
Notifications
You must be signed in to change notification settings - Fork 9
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
IS-322 Setup GrowthBook for FE #1449
Conversation
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.
minor comments
src/utils/growthbook.ts
Outdated
return new GrowthBook({ | ||
apiHost: GROWTHBOOK_API_HOST, | ||
clientKey, | ||
enableDevMode: isDev, // enable only for local dev |
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.
this is never set to true
- wdyt about removing this argument from getGrowthBookInstance
and setting it based on whether REACT_APP_ENV === LOCAL_DEV
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 thought about that but in our local dev, our react env is not LOCAL_DEV as per 1PW
Also the intended use is that if devs need this, they add the true
to the constructor in App.jsx and by default it will remain false
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.
hmm, my main worry is that this value should be true by default. this will result in q abit of overhead when switching between branches, for example (have to stash, switch, pop etc), which is why i thought it might make sense to dynamically compute this on a per env basis.
the tradeoff we're making here seems to be the time taken to standardise REACT_APP_ENV
versus how often this might be swapped to true
.
if it makes sense for this to be false by default, feel free to resolve this comment. otherwise, maybe just put a message out in the slack channel to standardise this would be ok toO!
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.
@seaerchin This option should definitely be false by default to prevent accidental commits or pushes which may pose a higher risk. The docs state:
To avoid exposing all of your internal feature flags and experiments to users, we recommend setting this to false in production in most cases.
I wanted the act of turning this on to be a conscious choice.
But if you are worried about the manual overhead and prevent a more dynamic setting, we could just do this:
export const getGrowthBookInstance = (clientKey: string) => {
// disable for staging and production
const isDev =
process.env.REACT_APP_ENV !== "staging" &&
process.env.REACT_APP_ENV !== "production"
return new GrowthBook({
apiHost: GROWTHBOOK_API_HOST,
clientKey,
enableDevMode: isDev, // enable only for local dev
})
}
Wdyt?
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.
oh cos that's the production flag - so imagine if you always want to see the new dev stuff, you'd always need to stash then pop
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.
more inclined to do what you've laid out above - with the only caveat being that we align localDev
cos got (non dev) envs that we might wanna have - eg: uat/vapt
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.
Yea exactly, so we need to be firm on the definition and there could be future cases where we introduce new environments. To be on the safe side, the flag needs to be a whitelist (assert if specific environment exists) rather than asserting some environment does not match
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.
can, sounds good! i'll approve it first - feel free to merge once isDev
flag is in
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.
added
d8cfd16
to
95d97be
Compare
* docs(editable): add docs for drag drop stuf (#1445) Co-authored-by: seaerchin <[email protected]> * fix(editable): shift out contxt to allow nested dnd (#1448) Co-authored-by: seaerchin <[email protected]> * feat: introduce new version of NotFoundPage (#1435) * Add submarine image for NotFoundPage * Add new version of the NotFoundPage * Add storybook stories for new NotFoundPage * Fix unit test for RouteSelector * fix(ui): changes based on design feedback * IS-322 Setup GrowthBook for FE (#1449) * feat: add growthbook as dep * feat: add growthbook to app * feat: inject user attributes * feat: add default dev mode to be false * feat: inject site name as attribute * feat: add types and featur flag list * chore: minor fixes * feat: add type and conditional siteName attribute * fix: enable dev mode only for local dev * chore: remove duplicate comment * IS-417 - Fixing Selector Issues in E2E tests (#1453) * feat: update text assertion * fix: resources text assertion * fix: method to remove other collaborators * fix: uncomment code * feat: extract constant * feat: swap removeFirstCollaborator with removeOtherCollaborators * fix: use removeOtherCollaborators * chore: remove unused import and function * chore(edithomepage): shift constants out (#1416) * refactor(edithomepage): use new constants * refactor(edithomepage): shift to new folder * refactor(edithomepage): shift preview out into own section --------- Co-authored-by: seaerchin <[email protected]> * IS-508: Introduce Interface for GrowthBook (#1455) * feat: introduce interface, remove constants file * fix: comment out sample flags * ref(contactus): stylings for contact us page editor (#1458) * Enhance AddSectionButton to allow adjusting button text * Add various sections of contact us page * Adjust Editable to allow new droppable zones * Update validators to match Figma messages * Introduce feature-flagged contact us page * Remove unneeded comment * Switch to use editable contexts instead and use modal * Fix validation condition * Fix name for FeatureFlaggedContactUs * Remove some unnecessary casts * Fix RouteSelector unit test * chore: set pointerEvents to none for draggable icon button (#1456) * Feat/is 443 nav bar styling (#1457) * chore: update editable types to allow nav * feat: add pageMenuBody * feat: add resourceMenuBody * feat: add folderMenuBody * feat: add GroupMenuBody * feat: swap new components into editNavBar * chore: fix text for components * Fix: addSectionButton button text prop * chore: pass props to button instead of box * chore: add feature flag for new nav bar (#1459) * chore: add feature flag for new nav bar * fix: tests * feat(feature-flag): add feature flag from gb (#1462) * feat(feature-flag): add feature flag from gb * refactor(navbar): use gb for ff * refactor(feature-flag): add types --------- Co-authored-by: seaerchin <[email protected]> * fix(edithomepage): remove optional tags (#1450) Co-authored-by: seaerchin <[email protected]> * Feat/is 494 smooth scroll (#1439) * refactor(edithomepage): smooth scroll * fix: only scroll after first load * chore: fix rebase errors * Revert "fix: only scroll after first load" This reverts commit 158a4df. * Revert "refactor(edithomepage): smooth scroll" This reverts commit e555c2f. * fix: firstLoad condition * chore: move first load check to separate method * fix: smooth scroll --------- Co-authored-by: seaerchin <[email protected]> Co-authored-by: Alexander Lee <[email protected]> * Fix/homepage images on private repos (#1461) * chore: add util method to retrieve image path * fix: replace image retrieval in homepage template components with updated v2 endpoint * fix: change return of util method to handle nested * Update src/utils/images.ts Co-authored-by: Qilu Xie <[email protected]> * Update src/utils/images.ts Co-authored-by: Qilu Xie <[email protected]> * chore: fix styling * refactor: move image fetching to separate hook --------- Co-authored-by: Qilu Xie <[email protected]> * 0.41.0 --------- Co-authored-by: seaerchin <[email protected]> Co-authored-by: seaerchin <[email protected]> Co-authored-by: Harish <[email protected]> Co-authored-by: Hsu Zhong Jun <[email protected]> Co-authored-by: Alexander Lee <[email protected]> Co-authored-by: Qilu Xie <[email protected]>
Problem
We are currently feature flagging using env variables which can get messy as we rollout more features. We also do not have an easy way to do things like percentage rollouts.
Closes IS-322
Solution
This PR introduces GrowthBook as a feature flagging system. It sets up some basic code that intialises the SDK and auto-fetches values when they change. It also sets user attributes to the SDK so that we can perform experiments in future.
Note that attributes are not sent to the cloud but rather stay within the local app.
Source: https://docs.growthbook.io/features/targeting
We also introduce a constants file which mirrors the FE-related flags as it is on GrowthBook for consistent referencing.
Breaking Changes
Tests
Part 1 - Basic Usage
useGrowthBook
hook to get the GB instanceuseFeatureIsOn
oruseFeatureValue
hooks to retrieve the created flagPart 2 - Attribute Setting
Deploy Notes
New environment variables:
REACT_APP_GROWTHBOOK_CLIENT_KEY
: this is required for GrowthBook SDK to connect with the cloudNew dependencies:
@growthbook/growthbook-react
: GrowthBook SDK