-
Notifications
You must be signed in to change notification settings - Fork 5
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
refactor(extension): use isFeatureEnabled #775
base: main
Are you sure you want to change the base?
Conversation
@@ -10,25 +10,24 @@ TOKEN_PRICE_POLLING_IN_SEC=300 | |||
SAVED_PRICE_DURATION_IN_MINUTES=720 | |||
|
|||
# Feature Flags | |||
USE_PASSWORD_VERIFICATION=false | |||
USE_ADA_HANDLE=true |
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 only sorting. It would be good to have such things sorted so it's easier to manage
|
||
export const POSTHOG_ENABLED = process.env.USE_POSTHOG_ANALYTICS === 'true'; | ||
export const POSTHOG_ENABLED = isFeatureEnabled('POSTHOG_ANALYTICS'); | ||
export const POSTHOG_OPTED_OUT_EVENTS_DISABLED = process.env.USE_POSTHOG_ANALYTICS_FOR_OPTED_OUT === 'false'; | ||
export const PUBLIC_POSTHOG_HOST = process.env.PUBLIC_POSTHOG_HOST; |
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.
If you like this approach, I would like to do similar refactoring to address accessing env vars like getEnvironmentVariable
process.env.CARDANO_SERVICES_URL_PREVIEW = 'https://preview-prod.com'; | ||
process.env.CARDANO_SERVICES_URL_PREPROD = 'https://preprod-prod.com'; | ||
process.env.CARDANO_SERVICES_URL_MAINNET = 'https://mainnet-url.com'; | ||
require('dotenv-defaults').config({ |
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.
Use .env
file with fallback to .env.defaults
instead of manually maintained list of feature flags and env vars
Allure report
smokeTests: ❌ test report for 6957e7dd
|
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Proposed solution
I'm proposing a new (slightly updated) way of using feature flags in the browser extension code base.
Instead of reaching out directly in code to
process.env
, I've created a helper methodisFeatureEnabled
which is an abstraction layer on top ofprocess.env
. It does the same thing, but in more maintainable way. In the future if we decide to use other solution for feature flags it will be easier to refactor.Apart of easier usage and better semantic there is one more benefit of this approach.
isFeatureEnabled
throws Error in case feature flag is not defined in.env
.Ex. Code is using
USE_HIDE_MY_BALANCE
feature flag, but it is not defined in jest's config fileset-env-vars.js
Additionally I configured jest to use the same
.env
file as engineer working with the code. In case there is no.env
then.env.defaults
will be used.How to use this helper?
.env.defaults
and.env.example
(please keep flags sorted) ex.USE_MY_FEATURE=true
FeatureFlag
type inutils/feature-flags.ts
(please keep flags sorted) ex.MY_FEATURE
process.env.USE_MY_FEATURE === 'true'
useisFeatureEnabled('MY_FEATURE')