launchdarkly / js-client-sdk

LaunchDarkly Client-side SDK for Browser JavaScript
Other
112 stars 65 forks source link

[React SDK] Expose a way to hook into LD Connection Errors #175

Closed martinmckenna closed 4 years ago

martinmckenna commented 5 years ago

The Problem

Currently, our app that relies on the LaunchDarkly service is render blocked by the actual LD Service. In other words, if the LD Service fails to connect, our app will never load.

Specifically if this line fails, we never know.

This is in a nutshell how we use the React SDK

import React from 'react'

export const App: React.FC<Props> = props => {
  const { userID } = props;

  /** PROBLEM: THIS MAY NEVER INIT AND BE UNDEFINED FOREVER */
  const client = useLDClient();

  const [isLoading, setLoading] = React.useState<boolean>(true)

  React.useEffect(() => {
    if (client && userID) {
      client
        .identify({
          key: userID
        })
        /**
         * set loaded to true if client fails or succeeds.
         * 
         * We don't want to render block if the flags can't be loaded.
         */
        .then(() => setLoading(false))
        .catch(() => setLoading(false));
    }
  }, [client, userID]);

  if (isLoading) {
    return null;
  }

  return <App />;
};

export default App;

but the issue here is that we have no way of knowing whether or not the connection succeeded.

Ideal Solution

Provide a new prop, maybe called ldConnectionError to the <WrappedComponent /> here..

So that could open a consumer component up to the following possibilities

import React from 'react'

export const App: React.FC<Props> = props => {
  const { userID } = props;

  const [client, clientError] = useLDClient();

  const [isLoading, setLoading] = React.useState<boolean>(true)

  React.useEffect(() => {
    if (client && userID) {
      client
        .identify({
          key: userID
        })
        /**
         * set loaded to true if client fails or succeeds.
         * 
         * We don't want to render block if the flags can't be loaded.
         */
        .then(() => setLoading(false))
        .catch(() => setLoading(false));
    }
  }, [client, userID, clientError]);

  if (isLoading && !clientError) {
    return null;
  }

  return <App />;
};

export default App;
bwoskow-ld commented 5 years ago

Hi @martinmckenna,

What kind of connection failure are you seeing? For example, is your connection error unrecoverable such as a 401 / 403 error (example: using an invalid client-side ID) or recoverable (example: a polling request was attempted and failed)? Based on reading the code we expect it to be the latter.

The current behavior is such that the React SDK will declare itself as "ready" when it has either successfully initialized or had an unrecoverable error. No "ready" event is fired if neither of these happen, which we expect may happen if you have a series of consecutive recoverable errors.

martinmckenna commented 5 years ago

Hey @bwoskow-ld - thanks for the response.

I was actually talking about more of the former (although I'd like to fail gracefully regardless). There's a couple instances where I fear that the LaunchDarkly service could render-block our app. For example

  1. If for whatever reason, we forget to pay the invoice and our service is shut down.
  2. If we have a build error and the client id environment variable isn't injected into the app correctly.

In these cases, const client = useLDClient(); is undefined and everything within the if statement in the example will never execute, causing our app to be in an infinite loading state.

I know the answer to this solution is probably "just use the vanilla JS SDK" but really, I'd love to be able to listen for connection failures the React way.

Just to be clear, this isn't actually an issue affecting any of our customers at the moment, but the idea is that I'd like to prevent this scenario from ever happening.

eli-darkly commented 5 years ago

@martinmckenna To be clear, is this actual behavior you've observed— or a theory of what would happen if the environment ID was invalid? If it's the latter, that should be easy to test.

The documentation is not clear on this point but based on my reading of the code, I would not expect it to hang in that case. I would expect that LaunchDarkly would return a 401 error, which would cause the JS SDK client to enter a failed state. In this failure case, the JS SDK fires an event ("ready") to indicate initialization is complete, just as it would on successful initialization. That event is what the React SDK listens for, so it should not continue to hang in that case. The ldClient should not be null, it should be a client object that returns default values for flags. That was the intended design, so if the behavior is otherwise this is a bug.

martinmckenna commented 5 years ago

@eli-darkly @bwoskow-ld With this code:

export default withLDProvider({
  clientSideID: undefined!,
  /**
   * Initialize the app with an anonymous user.
   */
  user: {
    key: 'anonymous',
    anonymous: true
  }
});

I see this in the console

Screen Shot 2019-08-28 at 3 09 33 PM

This is even before I invoke the useLDClient React hook.

I realize that sending no clientSideID is a clear issue, but I'd at least like to be able to listen to and gracefully catch that error.

eli-darkly commented 5 years ago

Ah - if you're not setting the environment ID at all, yes, that's a different problem that currently does not cause the client to become ready. The error is coming from the JS SDK code and I don't think there is currently a way for the React SDK code to get at it. I'll have to look into this a bit more to see the best way to address it.

eli-darkly commented 4 years ago

I've moved this issue over to the react-client-sdk repository: https://github.com/launchdarkly/react-client-sdk/issues/4