Closed harishv7 closed 1 year ago
my main problem with this approach is that suddenly alot of middleware ignorant stuff suddenly has to become middleware-aware
this leads to our codebase being resistant to change - if for every new feature, i'd have to pass
sessionData
down 3-4 layers, it gets very untenable very quickly. this is already felt most clearly when we have a nested router, and i don't want to make the problem worse than it already is.given that this is already approved, i don't have an issue with it functionality-wise. maintainability-wise, i feel like the more direct approach of just having a global singleton might be the best - we don't actually need to pass it
clientKey
togetNewGrowthbookInstance
as it's an env var + this allows us to trim away the deps passing ofsessionData
@seaerchin Isn't sessionData already being passed down? It's analogous to the context
of a request which is short-lived till that operation finishes
just having a global singleton might be the best
The SDK is the singleton, every time we do a new GrowthBook(), behind the scenes it is actually using a singleton.
We need to instantiate a new "wrapper" over this singleton for every request since it belongs to the session of the user. It should be tied to the user's request. Hence, think of it as being part of the session
my main problem with this approach is that suddenly alot of middleware ignorant stuff suddenly has to become middleware-aware this leads to our codebase being resistant to change - if for every new feature, i'd have to pass
sessionData
down 3-4 layers, it gets very untenable very quickly. this is already felt most clearly when we have a nested router, and i don't want to make the problem worse than it already is. given that this is already approved, i don't have an issue with it functionality-wise. maintainability-wise, i feel like the more direct approach of just having a global singleton might be the best - we don't actually need to pass itclientKey
togetNewGrowthbookInstance
as it's an env var + this allows us to trim away the deps passing ofsessionData
@seaerchin Isn't sessionData already being passed down? It's analogous to the
context
of a request which is short-lived till that operation finishesjust having a global singleton might be the best
The SDK is the singleton, every time we do a new GrowthBook(), behind the scenes it is actually using a singleton
I clarified the part on singleton with GB team:
"Our JS and React SDKs create a shared, cached singleton across all SDK instances and each new SDK instance is just a thin, short-lived wrapper around that singleton. This allows each SDK instance to be user-specific (based on attributes) while still tapping into the central feature repository."
@seaerchin Isn't sessionData already being passed down? It's analogous to the
context
of a request which is short-lived till that operation finishes
tbh context
is ok but the stuff that we're passing sessionData
too seems too deeply nested to be aware of context (session data here). it should only be that our topmost layer (routes) are aware of the context, extract necessary values and passes them on. if we can either 1. make growthbook standalone or 2. extract required stuff from growthbook, i'm ok
The SDK is the singleton, every time we do a new GrowthBook(), behind the scenes it is actually using a singleton.
We need to instantiate a new "wrapper" over this singleton for every request since it belongs to the session of the user. It should be tied to the user's request. Hence, think of it as being part of the session
yep, i think growthbook is structured as class instance (pass properties to segment) -> acc instance
fyi, got build errors - might wanan fix prior to merge
fyi, got build errors - might wanan fix prior to merge
fixed
@seaerchin refactored methods to have finer depedencies
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-310
Solution
This PR introduces GrowthBook as a feature flagging system. It sets up some basic code that intialises the SDK and adds
growthbook
as an req property. Each time we do this, it creates a new instance of GrowthBook.Note that
growthbook
can also be imported directly into the file/service where required too. The above req property injection helps us to perform conditional handling starting at controller level.Breaking Changes
Tests
-[ ] Create a feature flag on GrowthBook - either boolean or string or object -[ ] Use the
isOn
orgetFeatureValue
SDK functions inreq.growthbook
object to retrieve the created flag -[ ] Perform a conditional flow in one of the routes and see the behaviourDeploy Notes
Please remove existing env var of
WHITELISTED_GIT_SERVICE_REPOS
from EB.Deprecated environment variables:
WHITELISTED_GIT_SERVICE_REPOS
New feature flags
ggs_whitelisted_repos
: ensure correct values are set on GB portalNew environment variables:
GROWTHBOOK_CLIENT_KEY
: required for SDK to connect to GrowthBookNew dependencies:
@growthbook/growthbook
: GrowthBook SDKcross-fetch
: Polyfill for client side libseventsource
: Polyfill for client side libs