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] React SDK: Simplify bootstrapping of the OpenFeatureProvider #1047

Open
wichopy opened this issue Oct 20, 2024 · 6 comments
Open
Labels
enhancement New feature or request question Further information is requested

Comments

@wichopy
Copy link
Member

wichopy commented Oct 20, 2024

Requirements

This issue has similarities to #968

I am used to seeing the initialization of React providers being very minimal. Some examples are react router and react-query. They require passing in a config and you're done. Internally they could be setting some global state in some core library, but the developer doesn't have to think about that.

With the open-feature react SDK, you need to set the openfeature global object prior to using the provider.

Proposed interface change to the README:

-// Instantiate and set our provider (be sure this only happens once)!
-// Note: there's no need to await its initialization, the React SDK handles re-rendering and suspense for you!
-OpenFeature.setProvider(new InMemoryProvider(flagConfig));

// Enclose your content in the configured provider
function App() {
  return (
-    <OpenFeatureProvider>
+    <OpenFeatureProvider provider={provider}>
      <Page></Page>
    </OpenFeatureProvider>
  );
}

+//For domain scoped providers
+function App() {
+  return (
+    <OpenFeatureProvider provider={provider} domain={domain}>
+      <Page></Page>
+    </OpenFeatureProvider>
+  );
+}

If we were to do go forward with a change like this, it would probably need to be in the next major release?

@wichopy wichopy added the enhancement New feature or request label Oct 20, 2024
@beeme1mr
Copy link
Member

The default provider is always the same since we're using a global singleton. For that reason, I think it might be misleading to explicitly register an OpenFeature provider in the React provider configuration. However, we could potentially create the behavior you're proposing by leveraging a domain behind the scenes.

This is just spitballing, but we might be able to do something like this:

function App() {
  return (
    // Automagically domain scopes this provider.
    <OpenFeatureProvider provider={provider}>
      <Page></Page>
    </OpenFeatureProvider>
  );
}

The main advantage would be that all providers are scoped appropriately. However, it may introduce unnecessary complexity for both us, the SDK maintainer and the end users.

What do you think @toddbaert @wichopy @lukas-reining @thomaspoignant .

@lukas-reining
Copy link
Member

The default provider is always the same since we're using a global singleton.

Yes for the default provider this would lead to strange behavior as @beeme1mr says.

However, we could potentially create the behavior you're proposing by leveraging a domain behind the scenes.

This could be an idea. This feels like a OpenFeature internal use of domains.

@beeme1mr
Copy link
Member

It would be nice to have something in the React Provider that reminds users to configure a provider. Perhaps we could define a pattern that doesn't imply any provider scoping.

@lukas-reining
Copy link
Member

On the other hand one could argue that what @wichopy proposed fits perfectly to what the context mutator does.
There the domain is also implied from the context and transparent to the caller of mutateContext.
So for consistency we should keep these in sync maybe. What do you think @toddbaert @beeme1mr ?

@wichopy
Copy link
Member Author

wichopy commented Oct 22, 2024

My reasoning is since we are trying to Reactify things, developers shouldn't need to interact with the OpenFeature singleton directly. A React developer should come in and use the openfeature lib like they would any other lib, which is through hooks and components. I'm still familiarizing myself with how the web sdk works, but how is having the react provider set the openfeature provider an issue? I imagined just moving the OpenFeature.setProvider into the react-sdk.

So something like this:

export function OpenFeatureProvider({ client, domain, provider, children, ...options }: ProviderProps) {
  // some assertions that provider is not already set and that the provider is valid
  // In the readme we state that the developer needs to be careful not to set the provider more than once, why cant we do that for them?
  if (validateArgs(provider) {
    // throw errors
  }

//  if (!client) {
//    client = OpenFeature.getClient(domain);
//  }

  OpenFeature.setProvider(domain, provider);
  const client = OpenFeature.getClient(domain);

  return <Context.Provider value={{ client, options, domain }}>{children}</Context.Provider>;
}

@beeme1mr
Copy link
Member

Hey @wichopy, I agree that it isn't a technical problem. I'm more concerned about implying scoping behavior that doesn't exist.

It could also lead to odd configurations like:

<OpenFeatureProvider provider={Provider1} >
  <Header />
  <OpenFeatureProvider provider={Provider2} >
     <AnotherComponent />
  </OpenFeatureProvider>
</OpenFeatureProvider>

This would lead to unexpected behavior.

However, I can see the value of something like this, and I agree that we should make this React-friendly. @lukas-reining @toddbaert, what do you think?

@beeme1mr beeme1mr added the question Further information is requested label Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants