Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature Request] Support Suspense #192

Open
tjosepo opened this issue Apr 24, 2024 · 4 comments
Open

[Feature Request] Support Suspense #192

tjosepo opened this issue Apr 24, 2024 · 4 comments

Comments

@tjosepo
Copy link

tjosepo commented Apr 24, 2024

Hi,

I hope you are having a great day where you are.

I've been trying the React SDK, and there's something I think SDK could really benefit from.

Problem

Currently, all the hooks of the client require the consumer to wait until the client is ready before trying to use their values.

This forces consumers of the library to code very defensively and add checks to isReady in every single component that uses Split.

Some use-cases, like setting a default value from a feature flag, become very difficult because of this.

function Component() {
  const { treatments, isReady } = useSplitTreatments({ names: ["isDefaultEnabled"] });
  
  // 💫 Oops! We forgot to check `isReady`! This inital value might not be what we expect!
  const [enabled, setEnabled] = useState(treatments["isDefaultEnabled"] === "on");
 
  // ...
}

Proposal

Suspsense data-fetching could solves this problem by making the component suspend until the client is ready. This way, the user never has to check if the client is ready -- if the component successfully renders, the client is guaranteed to be ready.

This could be implemented in a new hooks, called useSplitTreatmentsSuspense.

function Component() {
  const { treatments } = useSplitTreatmentsSuspense({ names: ["isDefaultEnabled"] });
  
  // If the component didn't suspend here, it means the client is ready, and we can use treatments directly.
  
  // ✅ This becomes valid!
  const [enabled, setEnabled] = useState(treatments["isDefaultEnabled"] === "on");
 
  // ...
}

Under the hood, the useSplitTreatmentsSuspense would throw a promise and suspend when the client is not ready. This promise would resolve once the client becomes ready, resuming the rendering of the component.

Unfortunately, there is currently no way to implement Suspense data-fetching in user-land when using the React SDK. This is because the Split client is completely hidden from the consumers of the library until the client is ready, preventing the user from listening to the activation event of the client and implementing useSplitTreatmentsSuspense themselves. This forces users who want to use Suspense to fallback to the JavaScript SDK.

Let me know what you think about this proposal, and let me know if I can help with anything.

@agustinona
Copy link

Thanks @tjosepo for the suggestion. Our team will review this.

This is because the Split client is completely hidden from the consumers of the library until the client is ready

Could you clarify what you mean here? You can use the useSplitClient hook to get the underlying JS SDK client and make use of its events. You may also create a JS SDK factory instance using the SplitSdk function (as described in the For more flexibility section here) to leverage the the JS SDK factory while still using the React package.

Do neither of these approaches allow you to implement your suspense hook?

@tjosepo
Copy link
Author

tjosepo commented May 1, 2024

You can use the useSplitClient hook to get the underlying JS SDK client and make use of its events.

The useSplitClient() hook returns { client: null } until the client is ready. This prevents listening to the SDK_READY event, since the client is only accessibly once it's ready. Without being able to listen to the SDK_READY event, we can't use Suspense, because we have no way to know when to resume rendering.

I don't know if this is by design or if this is a bug in the library. If useSplitClient() returned the client during initialization, it would actually let users implement their own Suspense hook, but because it doesn't, users can't.

That being said, I also think there is value in officially supporting Suspense in the SDK, rather than require users to create their own custom hooks given that Suspense is a first-class feature in React.

@EmilianoSanchez
Copy link
Contributor

EmilianoSanchez commented May 7, 2024

Hi @tjosepo ,

You are right. When SplitFactoryProvider is called with the config prop, rather than the factory prop, the client is null on the first render. This was done this way by design because we have to avoid side-effects in the render phase, as React component rules state. So the Split factory is created when the component did mount, and therefore the client is not available during initialization (1st render).

This is something that we will review for next breaking change release for sure, to make the client available on initialization and support Suspense. But we need to discuss it with the team, for instance, to decide if we should provide a new hook as your proposal, or a new suspense: true option for useSplitTreatments, etc. So, we don't have any ETA yet.

In the meantime, you can consider using the factory prop with the SplitSdk function mentioned by @agustinona, described in the For more flexibility section here.

Using the factory prop, client is always available. So the useSplitTreatmentsSuspense hook could be implemented as follows:

import { SplitFactoryProvider, SplitSdk, useSplitTreatments, IUseSplitTreatmentsOptions } from '@splitsoftware/splitio-react';

function shouldSuspend(client: any) {
  const status = client.__getStatus();
  return !status.isReady && !status.isReadyFromCache && !status.hasTimedout;
}

function useSplitTreatmentsSuspense(options: IUseSplitTreatmentsOptions) {
  const result = useSplitTreatments(options);

  if(shouldSuspend(result.client)) {
    throw new Promise<void>((resolve) => {
      result.client!.once(result.client!.Event.SDK_READY_TIMED_OUT, resolve);
      result.client!.once(result.client!.Event.SDK_READY_FROM_CACHE, resolve);
      result.client!.once(result.client!.Event.SDK_READY, resolve);
    });
  }

  return result;
}

function MyComponent() {
  const { treatments } = useSplitTreatmentsSuspense({ names: [MY_FEATURE_FLAG] });

  return treatments[MY_FEATURE_FLAG].treatment === 'on' ? 
    <MyComponentOn /> : 
    <MyComponentOff />;
}

const mySplitFactory = SplitSdk(MY_SPLIT_CONFIG);

export const App = () => {
  return (
    <SplitFactoryProvider factory={mySplitFactory} >
      <Suspense fallback={<Loading />}>
        <MyComponent />
      </Suspense>
    </SplitFactoryProvider>
  );
}

And thanks for highlighting the importance of supporting Suspense :)

@EmilianoSanchez
Copy link
Contributor

For the record, we have released v2 of the React SDK, and the SplitFactoryProvider now makes the Split factory and client objects available from the initial render if you pass the config prop.

So, the implementation of the experimental useSplitTreatmentsSuspense should be updated as follows, to support the config prop:

function useSplitTreatmentsSuspense(options: IUseSplitTreatmentsOptions) {
  const result = useSplitTreatments(options);

  if (shouldSuspend(result.client)) {
    throw new Promise<void>((resolve) => {
      // @ts-expect-error `init` method is not part of the public API
      result.factory.init && result.factory.init();
      result.client!.once(result.client!.Event.SDK_READY_TIMED_OUT, resolve);
      result.client!.once(result.client!.Event.SDK_READY_FROM_CACHE, resolve);
      result.client!.once(result.client!.Event.SDK_READY, resolve);
    });
  }

  return result;
}

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

No branches or pull requests

3 participants