Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
IS-322 Setup GrowthBook for FE #1449
Changes from 8 commits
c046dc4
51d78b3
82e53d7
8d7ee9b
d1d4895
d0ccb2e
3676e12
95d97be
7ecbd11
28cbea8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 fromgetGrowthBookInstance
and setting it based on whetherREACT_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 falseThere 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 totrue
.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:
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:
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 inThere 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