Skip to content

Conversation

@martinmckenna
Copy link

@martinmckenna martinmckenna commented Jul 18, 2019

The Problem

Currently, there is no way (with the React Provider at least) to postpone the initialize method until you can ensure the user object you want to pass will be defined. This PR allows the client to programmatically wait until their user data has loaded before initializing the LaunchDarkly connection.

This is a huge pain point for my team right now as we try to integrate this React SDK into our large React/Redux app. There simply doesn't seem to be a way to hold off the initialization until we get our user data.

How it Works

In some file maybe named Parent.tsx

const Parent: React.FC<{}> = (props) => {
  const [username, setUsername] = React.useState<string | undefined>(undefined)

  React.useEffect(() => {
    /** request the profile then set in state */
    requestProfileData()
      .then(profile => {
        setUsername(profile.username)
      })
  }, [])

  return (
    <div>
      <h1>Some header text</h1>
      {/* only render the child when we have profile data */}
      {username && <Child username={username} /> }
    </div>
  );
};

export default Parent

Then in the child, we can be sure we have our profile data loaded and safely initialize the LD connection with our user data.

Child.tsx

import * as React from 'react';
import { compose } from 'recompose';

import { withLDProvider } from 'launchdarkly';

import { LAUNCH_DARKLY_API_KEY } from 'src/constants';

interface CombinedProps {
  username: string;
}

const Child: React.FC<CombinedProps> = (props) => {
  return (
    <div />
  );
};

export default compose<CombinedProps, CombinedProps>(
  React.memo,
  withLDProvider<CombinedProps>((ownProps) => ({
    clientSideID: LAUNCH_DARKLY_API_KEY,
    /** at this point, we know props.username is defined so initialization will work great */
    user: {
      key: `${ownProps.username}`
    }
  }))
)(Child);

flags: camelCaseKeys(bootstrap),
ldClient: undefined,
};
if (typeof config !== 'function') {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I'm not familiar enough with the localStorage option to see how this will work with this new logic.

@martinmckenna
Copy link
Author

Anyone out there to review this?

@yusinto
Copy link
Contributor

yusinto commented Jul 22, 2019

Hi @martinmckenna apologies for the delay. You don't have to delay initialize, what you should do is to initialize the react sdk without passing a user object at the root of your app:

// The user object is optional. If not specified, launchdarkly-react-client-sdk will create a default user 
// using a random guid
export default withLDProvider({ clientSideID: 'your-client-side-id' })(App);

Then somewhere else in your app when your user has logged in or when your app finally has your user data, use the native ldClient identify method to switch context to that user:

import React, { useState, useEffect } from 'react';
import { useLDClient } from 'launchdarkly-react-client-sdk';

const UserContextSwitchingDemo = () => {
  const ldClient = useLDClient();
  const [username, setUsername] = useState('null');

  useEffect(() => {
    requestProfileData().then(profile => {
      setUsername(profile.username);
      ldClient.identify({ key: profile.username }); // this will switch the user context
    });
  }, []);

  return <div>Hello {username}!</div>;
};

Refer to this similar issue #166 and the documentation on the ldClient identify function for more information.

Hope that helps!

@martinmckenna
Copy link
Author

martinmckenna commented Jul 23, 2019

@yusinto right that makes sense, but the issue here is that the LD connection will be initialized as an anonymous user correct? Which means that when we identify the user with the actual user data, another "active user" will be created. In other words, this will cost us more money long term, if I'm correctly understanding how monthly active users work.

My goal here is to just create 1 user per session, so that we don't creating too many MAUs and cut down on costs.

@eli-darkly
Copy link
Contributor

@martinmckenna: If you don't specify a user at all, then it will indeed create a new unique anonymous user. However, instead of creating a new unique user for each session, you can create a single throwaway user that will be shared across all sessions. You just need to specify a key with some value that you won't otherwise be using, and also set anonymous: true so that this user won't appear on your dashboard. For example:

export default withLDProvider({
    clientSideID: 'your-client-side-id',
    user: { key: 'null-user', anonymous: true }
})(App);

@martinmckenna
Copy link
Author

martinmckenna commented Jul 23, 2019

@eli-darkly I suppose.....that seems like a very hacky solution though and at that point, why even use the react sdk if the vanilla JS sdk can better work around this problem?

We're going to need to render-block the app regardless until the profile data is loaded anyway, so I'm just not understanding why it's not possible to pass user data directly to the HOC (either through my proposed solution or as a prop to the Provider). This seems like a pretty common convention with other libs that provide HOCs, such as the connect HOC that react-redux provides.

@eli-darkly
Copy link
Contributor

eli-darkly commented Jul 23, 2019

@martinmckenna: I can't comment on whether my proposed workaround is hackier than other approaches. I don't work with React myself, that is mostly Yus's thing; what I'm describing is the behavior of the underlying JS SDK, and it's an approach that I believe many of our customers use in JS, so I just thought I would mention it. It's certainly a much smaller code change. But I didn't say other solutions are not possible.

@martinmckenna
Copy link
Author

@eli-darkly fair enough. That's why this PR exists 😄

@martinmckenna
Copy link
Author

@yusinto is there any chance of this getting merged?

@martinmckenna
Copy link
Author

Is there anything I can do to get this moving forward? If it's not going to get merged, I'll close it, but I'd like to know one way or another.

LaunchDarklyCI pushed a commit that referenced this pull request Nov 6, 2019
add missing "secondary" attribute to TS definition for user
@yusinto
Copy link
Contributor

yusinto commented Nov 7, 2019

@martinmckenna apologies for the delay. We spent some time reviewing this internally. Our view is that the React SDK is designed to be initialized once at the beginning of an app's lifecycle and subsequently user context switches are supported through the ldClient object. This is the most common use case that we cater for and it's the one that we will definitely continue to maintain. We believe that initializing the SDK at a later time is a less common use-case and that Eli's suggestion of using the null user is a sound workaround. As a result, at this time we're going to decline these changes.

@bwoskow-ld bwoskow-ld closed this Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants