Defaulting to camel cased flags.

[trisrael]

I noticed multiple packages (splitio & LaunchDarkly) are converting incoming flags from their original form to be camel cased instead.

As a developer building out features using LaunchDarkly it seems to be easier to use the key supplied by the LaunchDarkly UI, copy & paste, instead of needing to camel case it before use.

I also did not see this documented anywhere. I lost a good amount of time with my flags returning false before digging into the library to see what was going on.

---

What's the philosophy surrounding the decision to camel case all incoming flags?

Maybe at the very least, we should add some documentation around this feature.

[tdeekens]

Sorry to hear that you spend time on this. I agree that we should have documented it better. The main reasoning back then was to have a flag name equal the value and camel cased flag names are eaier to work with as variable names. Then the removing the case change would have become a breaking change. The argument is not the best but I havent heard „complaints“ so far.

I see multiple take aways here

• [x] Imrpove the docs and inform about the case change better
• [ ] Consider a non-breaking config option on ConfigureFlopflip called shouldCamelCaseFlagNames defaulting to true

What do you think about that?

[tdeekens]

Added updated documentation here. The change to allow turning normalization on and off needs some discussion from here I think.

[trisrael]

Yah I like the shouldCamelCaseFlagNames option - I guess that would have to be supported by all the adapters though right?

[tdeekens]

Yeah it would be passed into the adaper. It‘s not really an adaperArg so I wouldnt put it there. So it would be optinal additional options each adapter has to accept.

[trisrael]

All right, well I'll wait for some discussion and manually override the adapter myself for now.

I started to look into the fix and found it was non-trivial for myself as a newcomer, by the number of references to ConfigureFlopFlip.

If it's as simple as updating configure in react-broadcast/react-redux for the new option, passing it to the adapters, splitio & launchdarkly, adding a few new tests for each, then I could probably do it.

While reading it, I also started to think that normalization might be configurable eventually, so passing normalizeFlag as a function might be interesting to think about.

In that same vein, shouldNormalizeFlagNames: true might be a more generic name for future proofing.

[tdeekens]

Glad to hear you‘re able to find your was around. Spend quite some time and thoughts in the organisation of the code. It feels like your evaluation is correct. Let me know if you want to spin up a branch or PR. Otherise no worries I can try to get to it some time too.

Function or boolean is a good question. I can‘t think of any other normalization type then from anything to camel case so maybe a boolean is enough.

[dferber90]

I don't have the full picture, but here's what I understood:

Users of flopflip sometimes check for the state of a feature-toggle by passing in non-camel-cased values (dasherized or underscored), e.g. through <ToggleFeature flag="dasherized-flag" />.

Since flopflip camel-cases all flags defined in toggle-services (LaunchDarkly, Split.io) automatically for convenience. But this can be confusing for library-users since they would expect the values defined in the toggle-service to match the ones passed to flopflip.

@tdeekens added a disclaimer to the docs pointing this behaviour out

I think we could do one better to prevent users from getting lost. What if <ToggleFeature /> and all other components accepting flag as an argument would warn when the user passes a dasherized or underscored flag? Assuming we always camel-case these, we would be able to know when to print the warning.

Alternatively we could do even more magic and apply the same conversion on the passed flags.

---

🐹 Now a less relevant rambling about default props:

In case we decide to go for an optional "config" prop, I'd invert it so that it's default value is false by changing the name: shouldNormalizeFlagNames -> skipFlagNormalization. Having opt-in props which default to false leads to nicer API's usually. In this case:

- <ConfigureFlopFlip shouldNormalizeFlagNames={false} />
+ <ConfigureFlopFlip skipFlagNormalization /> 

[tdeekens]

Yes, everything summarized above is the status of the dicussion.

Alternatively we could do even more magic and apply the same conversion on the passed flags.

I also like the approach more of normalizing passed flags while warning whenever they differ. It depends on how much people actually like to have the original flag names or a normalized version.

Obviously both approaches can also be mixed. Meaning that the skipFlagNormalization prop can also interop with the warning and passed flag normalization. If all components toggling and ConfiureFlopflip share configuration through the context.

[tdeekens]

Please refer to https://github.com/tdeekens/flopflip/pull/235 as an further improvement to the behaviour of the library.

[tdeekens]

Merged and released a new version of all affected packages.

[tdeekens]

@trisrael how are you coming along without this feature. Is this something which would still significantly easy your life working with the library or are you find with the 1. addition to the docs and 2. the addition of a warning in case of a casing mismatch?

[tdeekens]

Will close due to inactivity.

[trisrael]

Well, I had to monkey patch in order to get it to not camelcase, so that the server-side usage with Launchdarkly's node library would match our client-side usage with FlopFlip.

[tdeekens]

So you would still prefer having the normalization being an opt-out based feature using a skipFlagNormalization prop on ConfigureFlopFlip? It would imply a change there passing it into all adapters to respect that setting.

Would you mind spinning up a PR or how would you want to go about this?

[tdeekens]

This would be a 10 minute sketch of how it could work https://github.com/tdeekens/flopflip/pull/259

[trisrael]

Okay will do, probably won't get to it until next week.

[tdeekens]

Cool thanks. No rush. I created a fresh branch off master here called feat/skip-flag-normalization. Please swing the PR against it so we can collect changes there (with multiple PRs) and it's easier for me to help out.

[tdeekens]

I will close this now due to inactivity. It can be re-opened whenever a PR comes in.