avoid throwing in react hooks if the optimizely client is not available from the context

[fabb]

this allows an application to catch a thrown error from createInstance and use null as a fallback for the OptimizelyProvider, keeping the application working instead of crashing.

see #134 for more info

fixes #134

[fabb]

without this PR, using optimizely hooks without an <OptimizelyProvider> would throw

[zashraf1985]

Thanks for contributing. The way you are trying to fix it is by assuming that optimizely object passed in to Provider can be null and you have added checks to Provider and hooks to make sure they handle optimizely as null. Having worked on this SDK, i would fix it in a different way. I would make sure that createInstance from React SDK always returns the ReactSDKClient object. I would add safeguards inside ReactSDKClient to make sure it does not break if, for some reason, optimizely-sdk returns a null client. We are already picking it up and will fix it over the next few days separately.

[fabb]

@zashraf1985 sounds fine, that would be the 2. solution idea from #134

[fabb]

@zashraf1985 there is one downside to your approach for fixing the issue: testability. If the hooks would work without OptimizelyProvider, components that contain the hooks could be more easily unit tested. In my current unit tests I use a mocked ReactSDKClient with an OptimizelyProvider, but it‘s annoying to maintain because I need to implement all the functions from the interface, and it changes with updates of the lib (e.g. getIsReadyPromiseFulfilled was added lately).

[Develliot]

I'm with @fabb,

I think we should make the SDK less brittle. Possibly hooks shouldn't throw an error and fall over I think they should return an error as described here:

#152

I also think that the provider should just be a context provider with it's own its own useOptimiselyContext hook for getting and setting context. If setting the client and user id were separated either with their own functions or even when initialising context somehow, the provider should be able to exist without client being populated so if something goes wrong we can set it later. This will also prevent any rehydration issues with SSR and browser renders, if client fails or user id isn't ready.

import React, {
  FC,
  useState,
  createContext,
  useContext,
  Dispatch,
  SetStateAction,
} from "react";

import { ReactSDKClient } from "@optimizely/react-sdk";

export type OptimizelyContextStateType = {
  optimizely: ReactSDKClient | null;
  isServerSide: boolean;
  timeout: number | undefined;
};

const defaultState: OptimizelyContextStateType = {
  optimizely: null,
  isServerSide: false,
  timeout: 0,
};

export type OptimizelyContextProviderType = [
  OptimizelyContextStateType,
  Dispatch<SetStateAction<OptimizelyContextStateType>>
];

export const OptimizelyContext = createContext<OptimizelyContextProviderType>([
  { ...defaultState },
  () => {},
]);

export const useOptimizelyContext = () => useContext(OptimizelyContext);

export const OptimizelyContextProvider: FC = ({ children }) => {
  const [state, setState] = useState({
    ...defaultState,
  });

  return (
    <OptimizelyContext.Provider value={[state, setState]}>
      {children}
    </OptimizelyContext.Provider>
  );
};

you would then know it's totally safe to wrap your app in the provider because it just stores variables

any other file could do this sort of thing:

import { useOptimizelyContext } from "@optimizely/react-sdk";

 const [optimizelyContext, setOptimizelyContext] = useOptimizelyContext();
 const { optimizely } = optimizelyContext;



if (optimizely) {
  const user: UserType = {blah blah}
   optimizely.setUser(user)
 }

// or when initialising 
try {
  const client  =  await setUpClient();
  setOptimizelyContext({ ...optimizelyContext, optimisely: client })
} catch (err) {

}

[opti-jnguyen]

@Develliot - thanks for the suggestion! Will follow up on #152 regarding the useDecision hook fall over.

As for the provider change, though the change you suggested makes sense, we'd probably have to change the way we currently initialize the React SDK which may result in breaking changes for other users.

We'll continue discussing internally, but in the meantime I'd recommend posting the suggestion to our feature request board here and we'll prioritize it based on the number of votes it gets.

Thanks,
John

[Tamara-Barum]

Cleaning up old PR's - Seeing this has an ending comment, and merges tied to linked PR's- This can be closed.