launchdarkly / react-client-sdk

LaunchDarkly Client-side SDK for React.js
Other
81 stars 67 forks source link

LDProvider created by `asynWithLDProvider` may reset flags for partially React app #89

Closed zackgp closed 2 years ago

zackgp commented 2 years ago

Description We have a "partially" React app. "Partially" here means the React root node is a sub-node of the app's DOM tree.

AppRoot
|-- SubA
|-- SubB
|-- SubC
      |-- SubC1
      |-- SubC2
      |-- SubC3 (React root)

In this case, our SubC3 (React root) node will mount/unmount many times in our app's lifecycle.

The implementation of React root is almost like the code below:

import { asyncWithLDProvider } from 'launchdarkly-react-client-sdk';

let _LDProvider;

const getLDProvider = async () => {
    if (LDProvider) {
        return _LDProvider;
    }

    _LDProvider = await asyncWithLDProvider({
        clientSideID: process.env.LAUNCHDARKLY_CLIENT_ID,
        deferInitialization: true
    });

    return _LDProvider;
};

window.mountReact = async (reactRoot) => {
        const LDProvider = await getLDProvider();

        ReactDOM.render(
                <LDProvider>
                        // do ldClient.identify in side effects.
                        //...my react's content here and <LDConsumer> wrapped component of course. 
                       ...
                       <MyComponentWithLDConsumer />
                       ...
                </LDProvider>
        , reactRoot)
}

The problem comes out when any time our reactRoot re-renders.

For example:

class MyComponent {
       render() {
               const { flags, ldClient } = props;
               console.log(flags.featureA); // false
               console.log(ldClient.getAllFlags()['feature-A']); // true
               // ... renders the content
       }
}

export default withLDConsumer()(MyComponent)

Analysis

As dive into the implementation of asyncWithLDProvider, we find the problem might because the LDProvider use the flags fetched at the initialize time (const { flags: fetchedFlags, ldClient } = await initLDClient(clientSideID, user, reactOptions, options, flags);).

export default async function asyncWithLDProvider(config: ProviderConfig) {
  const { clientSideID, user, flags, options, reactOptions: userReactOptions } = config;
  const reactOptions = { ...defaultReactOptions, ...userReactOptions };
  const { flags: fetchedFlags, ldClient } = await initLDClient(clientSideID, user, reactOptions, options, flags);

  const LDProvider: FunctionComponent = ({ children }) => {
    const [ldData, setLDData] = useState({
      flags: fetchedFlags,
      ldClient,
    });

    useEffect(() => {
      if (options) {
        const { bootstrap } = options;
        if (bootstrap && bootstrap !== 'localStorage') {
          const bootstrappedFlags = reactOptions.useCamelCaseFlagKeys ? camelCaseKeys(bootstrap) : bootstrap;
          setLDData(prev => ({ ...prev, flags: bootstrappedFlags }));
        }
      }

      ldClient.on('change', (changes: LDFlagChangeset) => {
        const flattened: LDFlagSet = getFlattenedFlagsFromChangeset(changes, flags, reactOptions);
        if (Object.keys(flattened).length > 0) {
          setLDData(prev => ({ ...prev, flags: { ...prev.flags, ...flattened } }));
        }
      });
    }, []);

    return <Provider value={ldData}>{children}</Provider>;
  };

  return LDProvider;
}

When the first time LDProvider renders and ldClient.identify be called, the ldClient fetched the latest flags, it found the latest flags are different than the flags fetched at the initialize time, so the ldClient.on('change') callback will be called.

When the second time and after the LDProvider renders, no matter how many times ldClient.identify be called again, the ldClient will found it's flags are the same as the latest fetched flags, so the ldClient.on('change') callback will not be called again. The LDProvider's flags will fallback to fetchedFlags got at the initialize time.

Suggestion From our view, a minor change could fix this problem while still keeping the code semantically correct, that is:

  const LDProvider: FunctionComponent = ({ children }) => {
    const latestFlags = ldClient.getAllFlags();
    const [ldData, setLDData] = useState({
      flags: reactOptions.useCamelCaseFlagKeys ? camelCaseKeys(latestFlags) : latestFlags,
      ldClient,
    });
    // others will not change
    return <Provider value={ldData}>{children}</Provider>;
  };

Any time, LDProvider renders/re-renders, it will pop out the latest flags from the ldClient, so the LDProvider/LDConsumer will always got the latest flags no matter how many times it mounted/unmounted or renders/re-renders.

Expected behavior The flags extract from LDProvider/LDConsumer keeps the same as it in ldClient.

SDK version launchdarkly-js-client-sdk: 2.19.0 launchdarkly-react-client-sdk: 2.22.0

Not familiar with the issue format here, please let me know and I will fix it if I have something missed or wrong.

ctawiah commented 2 years ago

Hi @zackgp thanks for reporting the issue, let me dig into this a little further, I will get back to you before end of day. Thanks.

ctawiah commented 2 years ago

Hi @zackgp, I did my best to try and reproduce your issue but I couldn't successfully reproduce it (everything worked as expected), see my attempt below and let me know if i'm missing anything. I tried this using launchdarkly-react-client-sdk version 2.23.0 and 2.22.0.

const clientSideID = 'my-client-id';
let _LDProvider;

const getLDProvider = async () => {
    if (_LDProvider) {
        return _LDProvider;
    }

    _LDProvider = await asyncWithLDProvider({
        clientSideID: clientSideID,
        deferInitialization: true
    });

    return _LDProvider;
};

// Using function component to try and reproduce issue
const MyComponent = ({ flags, ldClient }) => {
   console.log(flags.exampleText); // true <--- expected
   console.log(ldClient.allFlags()['example-text']); // true. <--- expected

   return <div>This is a test from function component</div>
}
const WrappedComponent = withLDConsumer()(MyComponent)

// Using class component to try and reproduce issue
class MyClassComponent extends React.Component {
  render() {
    const { flags, ldClient } = this.props;
    console.log(flags.exampleText); // true <--- expected
    console.log(ldClient.allFlags()['example-text']); // true. <--- expected

    return <div>This is a test from class component</div>
  }
}
const WrappedClassComponent = withLDConsumer()(MyClassComponent)

const renderApp = async () => {
  const LDProvider = await getLDProvider();

  ReactDOM.render(
    <React.StrictMode>
      <LDProvider>
        <WrappedClassComponent />
      </LDProvider>
    </React.StrictMode>,
    document.getElementById('root')
  )
}

// I used setInterval here to try to simulate re-rendering multiple times as you explained
setInterval(() => {
  renderApp();
}, 5000);
zackgp commented 2 years ago

Hi @ctawiah , thanks for your attempt and I'm sorry that I mentioned one important thing very inconspicuous, that is: do the identify.

Please let me do a slight change to the MyClassComponent:

// Using class component to try and reproduce issue
class MyClassComponent extends React.Component {

  componentDidMount() {
    const { ldClient } = this.props;
    ldClient.identify({ key: 'test-user' })
  }

  render() {
    const { flags, ldClient } = this.props;
    console.log(flags.exampleText);
    console.log(ldClient.allFlags()['example-text']); 

    return <div>This is a test from class component</div>
  }
}
const WrappedClassComponent = withLDConsumer()(MyClassComponent)

And please also let the flag exampleText evaluated to false by default, and evaluated to true only for the user test-user.

By the way, could you please able to invite me to the test project you created for reproducing the issue on the LaunchDarkly system. (My LD account email address is: zack.qiu@globalpay.com)

ctawiah commented 2 years ago

Hi @zackgp, I modified the example with ldClient.identify({ key: 'test-user' }) but still couldn't reproduce the issue. I created a repo with the example I used and added you as an external contributor (please check your email for the invitation). Let me know if theres anything else I need to modify in the example to reproduce the issue.

zackgp commented 2 years ago

Hi @ctawiah, thanks a lot for the invitation and the example repo so I could clone it down and do some tests locally 🙏

The example code is almost close to the condition to reproduce the case. Since in our real system the SubC3 is frequently been removed and added back to the DOM tree, to simulate this condition, I added one line to the callback passed to setInterval:

const timer = setInterval(() => {
  ReactDOM.unmountComponentAtNode(document.getElementById('root')) // simulate the node been removed
  renderApp();
}, 5000);

Then, the issue comes out again! I guess you can have a try and will see it in this case. A lot of Thanks!

ctawiah commented 2 years ago

Hi @zackgp, thanks for providing the additional details, I was able to successfully reproduce the issue. Your suggested solution should help resolve it, we will need to make some small tweaks to it to filter targeted flags when specified. We will work on getting a fix out for this - I'll keep you posted when the fix is out. Thanks.

bwoskow-ld commented 2 years ago

This is resolved in version 2.23.1.