Children render callback

[HHogg]

Hi, would you be open to a PR that allows the render callback to be passed in as the children prop? This would allow people to keep the render logic together inside the render function.

The API I imagine it being like is something like...

render() {
  return (
    <FeatureFlag key="Feature1">
      { (value) => value 
        ? <Feature1Component /> 
        : <FallbackComponent />
      }
    </FeatureFlag>
  );
}

And this would also work for multivariant flags too...

render() {
  return (
    <FeatureFlag key="Feature1">
      { (value) => {
        switch (value) {
          case 1: return <Feature1Case1 />
          case 2: return <Feature1Case2 />
          case 3: return <Feature1Case3 />
          default: return <FallbackComponent />
        }
      } }
    </FeatureFlag>
  );
}

As FeatureFlag doesn't support any children at the moment, then it would not be a breaking change and could just be given the value.

I guess the change would be here, with something like ...

if (children) {
  return children(flagValue);
}

[sethbattin]

The proposal sounds fine, and the syntax change is definitely preferable.

I'm a little bit skeptical of being able to implement it with perfect back-compat. But no need to speculate; we might as well make the attempt. I think I would still lean toward calling it a major version number just to be slightly safer. It might simplify things to not attempt to keep it compatible, and just warn profusely about upgrading. Perhaps providing a codemod would be feasible.

There would be an existing use case that'd be a little tricky; it might not be feasible with a single-argument children-function. We have a scenario where rendering happens before any settings are loaded. Currently that is handled by initialRenderCallback prop. It would be awful to have to check type of a falsey value, say to different null/false/undefined. So perhaps that just implies a more complicated function signature for the children function. :man_shrugging: I don't know; see what you can do.

Obviously it would need doc updates too.

[HHogg]

Thanks for the feedback @sethbattin, I had not thought about the case before initial flags had been received. Is this what the checkFeatureFlagComplete flag is for inside FeatureFlagRenderer?

If so, perhaps we can pass that down also, something like.

render() {
  return (
    <FeatureFlag key="Feature1">
      { ({ flagInitialised, flagValue }) => {
        switch (flagValue) {
          case 1: return <Feature1Case1 />
          case 2: return <Feature1Case2 />
          case 3: return <Feature1Case3 />
          default: return flagInitialised  
            ? <FallbackComponent /> 
            : <InitialComponent />
        }
      } }
    </FeatureFlag>
  );
}

[andrewdushane]

I like the idea of this API, but it sounds like the existing API and a single children/render prop would need to be mutually exclusive. This could cause confusion, even if we attempt to document it.

On Wed, Oct 24, 2018 at 4:12 PM Harrison Hogg [email protected] wrote:

Thanks for the feedback @sethbattin https://github.com/sethbattin, I had not thought about the case before initial flags had been received. Is this what the checkFeatureFlagComplete https://github.com/TrueCar/react-launch-darkly/blob/master/src/components/FeatureFlagRenderer.js#L92 flag is for inside FeatureFlagRenderer?

If so, perhaps we can pass that down also, something like.

render() { return ( <FeatureFlag key="Feature1"> { ({ flagInitialised, flagValue }) => { switch (flagValue) { case 1: return <Feature1Case1 /> case 2: return <Feature1Case2 /> case 3: return <Feature1Case3 /> default: return flagInitialised ? <FallbackComponent /> : <InitialComponent /> } } } </FeatureFlag> ); }

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/TrueCar/react-launch-darkly/issues/85#issuecomment-432810961, or mute the thread https://github.com/notifications/unsubscribe-auth/AG_8aLqwzx6Jacgk5Ack-8ETbYfAZ0anks5uoMm3gaJpZM4X3_hT .

[HHogg]

That's understandable @andrewdushane, trying to provide 2 completely different ways to use the component does sound very messy. The reasons I prefer this, is that that it feels like a more familiar pattern, similar to other behavioural components like the new context API and React Router.

I'm happy to submit a PR that implements this alongside or instead of the existing API, or just create my own abstraction that allows me to do this. @sethbattin @andrewdushane what do you think is best, and would be happier with?