TrueCar / react-launch-darkly

Simple component helpers to support LaunchDarkly in your React app.
MIT License
76 stars 20 forks source link

Is there a way to disable the 'change' listener? #64

Closed peterbee closed 4 years ago

peterbee commented 6 years ago

FeatureFlagRenderer automatically creates a listener for feature flag changes. This can create a negative experience in some features (if the feature changes — or disappears — while a user is in the middle of doing something).

Would it make sense to have an option to disable this?

https://github.com/TrueCar/react-launch-darkly/blob/b99c6fd894bf593b186b8c799240dda5aee95d34/src/components/FeatureFlagRenderer.js#L81-L87

sethbattin commented 6 years ago

@peterbee can you elaborate a bit about your use case? Are the flag changes frequent, and what activities are they disrupting? Under what circumstances do you want to update flags (other than in-browser)? Are you using server rendering?

peterbee commented 6 years ago

We test multiple user experiences — and we don't want the UI to change while a user is using it (such as filling out a form or looking at search results). In most cases, we want our feature flags to stay the same for any given page load.

We are not bootstrapping the feature flag values from the server.

sethbattin commented 6 years ago

Ah, fair enough, I would have recommended the serverside-only solution of clientOptions.disableClient if that were part of your usage. Most of my experience with solving this issue is by using SSR to control the change. That, and features being either so small to be unnoticeable (default checked/unchecked), or so large that it would be impossible to swap them (whole routes). So this kind of middle ground is somewhat new territory, and the more details you can give, the better we can make this part of the library work.

In any case, it's clearly part of the API (or my own reference, the docs). So no reason to exclude it. Send a patch?

With my last question, I guess I was trying to ask about the timeframe over which a user would have page open, but definitely not want to see updates. Because it seems like there might be a tradeoff:

If the user has the page open for a long time, then you don't want to disrupt them during active use. But also if they load the page and keep it open for hours, perhaps they won't be actively using the features under test, and you would want hidden features to change when they're not in use. Say, the form is in a hidden modal, and it would be fine to update as long as the modal isn't open at that moment.

And on the other other hand, if the features under test are used only for moments one single time, or sporadically but repeatedly during the lifetime of the page, then perhaps it's a non issue. Because the chance of flag change intersecting active usage is very low relative to total time on the page. And in that case, the delay tradeoff and complexity is not worth the fact that the flag isn't deploying when you make admin updates. And if delayed updates are acceptable anyway, then why not hold them until off-hours and avoid the problem entirely?


Anyway! Does it make more sense to you to have this as a global setting or per-flag? for example:

<FeatureFlag flagKey={key} changeSubscribe={false}>`

^- that changeSubscribe prop could get a default true, and then users could opt-out for the fragile UI elements, but still get updates for other items.

peterbee commented 6 years ago

@sethbattin Thanks for walking through your use cases. It certainly raises a lot of other circumstances that are not easily resolved in the FeatureFlag component. Now I'm wondering if that component should have the responsibility of disabling the change listener.

As an alternative, I could use the HOC pattern to pass raw flag data into a child component and explicitly manage any flag updates there:

constructor(props) {
  super(props);
  this.state = { myFlag: props.flagKey };
}

componentWillReceiveProps(nextProps) {
  if (
    // only update state if LaunchDarkly flag was previously null (uninitialized)
    this.props.flagKey === null
    && nextProps.flagKey !== null
  ) {
    this.setState({ myFlag: nextProps.flagKey });
  }
}

Something like this would only update the state.myFlag value when LaunchDarkly initializes and any time the component mounts. (As you mentioned, if we wanted to allow flag updates at other times, that could also be easily managed in the child component.)

Here's an HOC based on the current FeatureFlag component:

export default flagKey => WrappedComponent => {
  const _renderFeature = function(props, featureFlagValue = true) {
    const flags = {};
    flags[flagKey] = featureFlagValue;
    return <WrappedComponent {...props} {...flags} />;
  };

  return function(props) {
    return (
      <FeatureFlag
        flagKey={flagKey}
        initialRenderCallback={_renderFeature.bind(this, props, null)}
        renderDefaultCallback={_renderFeature.bind(this, props, false)}
        renderFeatureCallback={_renderFeature.bind(this, props)}
      />
    );
  };
};
jacobmoretti commented 6 years ago

@peterbee How has your HOC been working out for you? I'm thinking going the route of what @sethbattin suggested is probably the best way forward: <FeatureFlag flagKey={key} changeSubscribe={false}>. Not entirely sold on the name of the prop but something akin to that. Curious to hear if that fits your use case or not. Also thanks for the feedback and submitting issues! Its helpful for us to get insight into how others are making use of this library. We have very specific use cases at TrueCar so we're always welcome to feedback and accepting pull requests.

peterbee commented 6 years ago

We haven’t made use of the HOC yet, so unfortunately I don’t have any feedback on that.

The changeSubscribe prop sounds like a fine idea!

sethbattin commented 4 years ago

Closing due to inactivity