Feature Flags!

[emptyrivers]

im not sure if this is actually a useful idea or not, but (assuming i didn't screw up the yaml) this would provide us a way to get users to opt in to new features without explicitly making them available to everyone who enabled alpha mode but didn't understand what that means. That way, stuff like 'oops we changed the export format for WeakAuras and now 2% of our users can never share their auras until the next release' would be avoidable.

It's also possible that we just tell those people to get back on the standard release channel, idk.

Experimental mode is automatically enabled for any working copy, or for build artifacts on pull requests, otherwise the user must send the command /wa optin to enable it.

Code can be feature-flagged by hiding it behind a if WeakAuras.IsExperimental() check.

[mrbuds]

That way, stuff like 'oops we changed the export format for WeakAuras and now 2% of our users can never share their auras until the next release' would be avoidable

Problem is we are not getting a lot of feedback/bugreport until a release is publish
When using this system i expect we get near zero feedback while new feature is still only available on beta

An alternative to your idea would be to have an experimental branch that is automatically (is it possible?) rebase on main, dedicated to PRs with an internalVersion bump, and merge it into main before a release
Or.. stop publishing beta on curseforge/wago

[Stanzilla]

The idea is cool but yeah I'm wondering if we'd get much use out of it. Potentially it could also be evolved into something like an A/B test system and also cover the different WoW flavor checks.

[emptyrivers]

Problem is we are not getting a lot of feedback/bugreport until a release is publish

When using this system i expect we get near zero feedback while new feature is still only available on beta

I don't think that's exactly what i'm imagining. it's more like, imagine if you had a big pull request that touched nearly every part of the codebase. such a big change naturally needs time to cook, but at the same time it's super annoying to have to solve merge conflicts all the time because normal maintenance keeps touching the same spots. It might be nice if changes could be committed more gradually & make the process smoother, meanwhile our LCD user is none the wiser. Once the feature is fully cooked, we would stop hiding it behind experimental status to "enable" the feature.

I would be open to changing the criteria for if a build is experimental - perhaps we just want to block the release channel from seeing experimental changes without optin, i'm not sure.

[InfusOnWoW]

I agree with rivers that it would sometimes be nice if we could ship features behind an opt-in method. Though I do think changing how auras behave can easily lead to unintuitive behaviour when auras are shared between users that have it enabled or not.

Even with such a flag available I probably couldn't use it for the progress source PR I have. Maybe it would have worked for the standby feature though.

Now there are some questions on how to enable/disable the experimental mode:
You did:

• Enable it in dev builds
• Enable it in pull requests builds

which I think are both fine.

I don't know about the method for user optin though, that is only enabled for the current session, so only temporarily.
And it requires code for experimental features to work with disabling/enabling experimental mode at any time. That feels potentially problematic.

[emptyrivers]

And it requires code for experimental features to work with disabling/enabling experimental mode at any time. That feels potentially problematic.

well, there's no way to disable experimental mode in a single session.😃 but, point taken; i was explicitly thinking about the case of "we are rolling out changes that involve a new export format, but we don't want to create a schism in our userbase", and a passive flag more than serves that purpose, since i doubt anyone is out there automatically exporting their auras

[Stanzilla]

Make it a generic feature flag system and we can try out different usecases?

[emptyrivers]

I whipped up a little feature flag system, which detects build type & can auto-enable named features based how WeakAuras was packaged. A feature can also declare itself to persist, and identify when/if an aura would require it to be enabled.

to use, call Private.Features:Register({id = "Foo", persist = true, autoEnable = {"dev", "pr", "alpha"}}), and then you can check Private.Features:Enabled(id) to branch code elsewhere in the codebase (Private.Features:Wrap(id, func) lets you do this with a bit of sugar). Alternatively, you can use Private.Features:Subscribe(id, callback) to listen for when a feature is toggled, so that you can set up/tear down frames & whatnot.

[Stanzilla]

That's pretty poggers as the youth would say

[emptyrivers]

Presumably we'd communicate to the user if an aura they had required a disabled feature to function properly, but that sounds touchy to do & i figured we would cross that bridge when we got there

[emptyrivers]

Hydration of feature flags is timed so that it should be the first thing that happens on PLAYER_LOGIN (the bit of code above is a saved variables migration that would only happen if a user played for a brief period in 2019 but then quit until now). So, even if a user opts into a persisted feature, the code will still briefly think the feature is disabled until PLAYER_LOGIN.

[emptyrivers]

I'm not sure exactly how to order the toc - since i'm using SubscribableObject obviously that needs to go before Features, but theoretically we could write feature flagged code at any point. So i just put it near the beginning, and we can muck with load order later if its a problem

[mrbuds]

My biggest concern with this PR is about features that require a data migration
Should features with a migration have an optional "not reversible" flag

• then show an explicit warning to use that they should do a backup
• and/or trigger Archivist save point?
• and/or possibility for an optional reverse migration?

[mrbuds]

It feels wrong to me that /wa optin is a toggle instead of having optin & optout

[emptyrivers]

I changed it to ff, for 'flag feature'. I don't care one way or another, if ppl want to bikeshed that I'm happy to change it to whatever people prefer.

[InfusOnWoW]

I agree with buds, enabling and disabling should be separate, otherwise telling users how to enable a feature is too complicated.

[mrbuds]

manually enable a feature may require a reload

if WeakAuras.IsLoginFinished() and self.__feats[id].enableBeforeLoginFinished then
  -- called from /wa optin and feature needs to be enable earlier
  ReloadUI()
else
  self.__feats[id].sub:Notify("ENABLED")
end

[emptyrivers]

Hmm, that depends on how code which depends on a feature is written, surely? I'd 'prefer' that code 'magically' works without having to reload ui if possible, if nothing else than for my personal convenience.

[emptyrivers]

[InfusOnWoW]

Otherwise looks reasonable. I'm pretty sure the disabling of features might not always be possible, but that can be adjusted once we have non-disabling features. And I'm a bit sceptical about whether auras requiring features is something we'll use, but imho with the two comments addressed it's ready to be merged.

[emptyrivers]

Last call for bikeshedding the syntax:

/wa feature action id

where action is one of toggle|enable|disable|on|off, and id must be some registered feature flag. Hopefully it's obvious what the actions do.
Failing to provide both action & id will print a help message.

[emptyrivers]

5 ='s, because bigwigs packager will use at most 4 ='s in its own @flag@ replacements

[Stanzilla]

Last call for bikeshedding the syntax:

/wa feature action id

where action is one of toggle|enable|disable|on|off, and id must be some registered feature flag. Hopefully it's obvious what the actions do. Failing to provide both action & id will print a help message.

works for me

[emptyrivers]

cherry picked this from the undo pull request; otherwise it's annoying to disable a feature which auto-enables on your build type

[InfusOnWoW]

Hmm, this leads to the feature being enabled at first, before being disabled by hydrate.

I think a simpler approach is to require:
a) Registering a feature must be done before Hydrate
b) Hydrate decides on whether a feature is enabled/disabled, by whatever logic you decide.
.
That centralizes the logic on whether a feature starts off as enabled/disabled in one place.

[emptyrivers]

I'm not sure if it actually is simpler, but I see the appeal.

[emptyrivers]

I reworked Hydrate() to be a barrier between registering features and setting their enable state. It's an error to try to register after hydration, or to enable before.