Skip to content

Feature flags#4582

Merged
koke merged 2 commits intodevelopfrom
try/feature-flag
Dec 17, 2015
Merged

Feature flags#4582
koke merged 2 commits intodevelopfrom
try/feature-flag

Conversation

@koke
Copy link
Copy Markdown
Member

@koke koke commented Dec 17, 2015

Instead of having a bunch of checks for DEBUG and INTERNAL_BUILD spread across the app, I suggest we keep them contained and actually check for enabled features.

This moves the checks from compile time to run time, so it might not be best for all scenarios:

  • Constants that have different values for each build
  • Static tables with row constants that depend on features, but we're moving those to ImmuTable progressively
  • Code that we don't just want disabled but not linked (e.g. HockeySDK)

Despite all this, I think this approach is interesting so we can easily start using more feature flags. Ideally we'd start work on a new feature by flagging it and enabling only on Debug builds. This way, we can make smaller PRs and only release the feature when it's ready.

I've only added the FeatureFlag code (+tests), I'll start using it on a separate PR

Needs Review: @SergioEstevao

koke added 2 commits December 16, 2015 17:56
FeatureFlag centralizes how we check for feature availability in a specific build
@koke koke added this to the 5.9 milestone Dec 17, 2015
@koke koke mentioned this pull request Dec 17, 2015
4 tasks
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the PR description you say that the checks pass from compile to run time, but this looks to be set on compile time?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that bit wasn't clear. The build type is still defined by build parameters, but using this code does the check at runtime. For instance:

#if DEBUG
  doStuff() // this isn't included in the final binary
#endif
if build(.Debug) {
  doStuff() // this is included in the binary, but it's never reached
}

@SergioEstevao
Copy link
Copy Markdown
Contributor

@koke I like the PR, but are we sure we want to be limited to the build type only? I know that in some parts of the app we where previous using a special URL scheme to activate features. Do you plan to support this?

@koke
Copy link
Copy Markdown
Member Author

koke commented Dec 17, 2015

are we sure we want to be limited to the build type only?

It's not limited. In FeatureFlag.enabled you can build your rules however you want. My first goal was to disable some stuff on App Store builds, so that's why the examples show that. But I'd also like to have some way to tweak those in the future, maybe even use flags that we get from the API for a specific wp.com account. I think this would still work as the frontend to all those checks.

@SergioEstevao
Copy link
Copy Markdown
Contributor

:shipit:

koke added a commit that referenced this pull request Dec 17, 2015
@koke koke merged commit 59028fe into develop Dec 17, 2015
@koke koke deleted the try/feature-flag branch December 17, 2015 12:37
@koke koke modified the milestones: 5.9, 6.0 Dec 18, 2015
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty looking tests ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants