open-feature / js-sdk

JavaScript SDK for OpenFeature
https://openfeature.dev
Apache License 2.0
143 stars 31 forks source link

React SDK implementation #545

Closed toddbaert closed 4 months ago

toddbaert commented 1 year ago

We should implement a React SDK. The SDK would provide a "React-friendly" API and features, built on top of the Web SDK.

:warning: Below is a tentative list of features we should implement (some points include questions, and all of these are up for debate) :warning:

Non-functional requirements:

Precedents:

moredip commented 1 year ago

I've sketched out what I think an idiomatic React API would look like here: https://github.com/moredip/openfeature-playground/tree/switch-to-web-sdk/libs/openfeature-hooks#readme

It's still a work-in-progress, but it's functional and illustrates the main parts of the API. You can also see example usage in a toy app here: https://github.com/moredip/openfeature-playground/blob/switch-to-web-sdk/apps/demo-react-weather-app/src/main.tsx https://github.com/moredip/openfeature-playground/blob/switch-to-web-sdk/apps/demo-react-weather-app/src/app/Weather.tsx#L55

The main thing I am advocating for is to use a react-query style API, where a single hook returns a rich result that you can then deconstruct to get both the basic feature flag value, as well as additional context behind the flag evaluation. This works really nicely in react-query, and I think it would work really nicely for feature flags too. (here's an example)

Different consumers will want to handle loading states differently

The main motivation with this style of API is ergonomic support for the different ways that consumers will want to handle non-authoritative states, like when a flag provider is still loading flag configuration. This happens a lot when a SPA is first loading, or during a logged out/logged in transition, where a flag evaluation will return a default value for a moment, then resolve to the actual authoritative value.

Some consumers will be happy to have a temporary "flash" while that default value is being used. Others will never want to use the default value, and prefer to show a loading UI or just not render at all until the authoritative flag state comes in. This is something that came up during our vendor interviews - different customers wanting to handle non-authoritative scenarios in very different ways.

the react-query style API makes this really easy - you just add some conditional logic based on flagResult.isAuthoritative, or similar.

lukas-reining commented 1 year ago

Really like your poc @moredip.

The main thing I am advocating for is to use a react-query style API, where a single hook returns a rich result that you can then deconstruct to get both the basic feature flag value, as well as additional context behind the flag evaluation. This works really nicely in react-query, and I think it would work really nicely for feature flags too. (here's an example)

This makes total sense to me, and we actually implemented it quite similarly at a client.

One question that is open to me is how we want to handle multiple named clients in one "component". Maybe one should be able to give the name of the OpenFeatureClient to the useFeatureFlag hook. E.g. this could be done by overloading useFeatureFlag. The context provider could possibly accept a list of clients.

// default client
const showDealOfTheDayFlag = useFeatureFlag('show-deal-of-the-day', false)

// named client
const showDealOfTheDayFlag = useFeatureFlag('my-client', 'show-deal-of-the-day', false)
moredip commented 1 year ago

One question that is open to me is how we want to handle multiple named clients in one "component".

@lukas-reining can you share a bit more about what might cause a consumer to need this? I can think of a few ways to handle it (your suggestion of an additional param to useFeatureFlags is a good one), but I'm also reluctant to complicate the initial version of an API to handle scenarios that aren't really needed.

lukas-reining commented 1 year ago

One question that is open to me is how we want to handle multiple named clients in one "component".

@lukas-reining can you share a bit more about what might cause a consumer to need this? I can think of a few ways to handle it (your suggestion of an additional param to useFeatureFlags is a good one), but I'm also reluctant to complicate the initial version of an API to handle scenarios that aren't really needed.

@moredip I think mostly this would be the case when you have two providers that you want to use. This could be because of different business domains which result in different teams and responsibilities. Or maybe simply because you have different systems you want to consume flags from. To me this should be the same argument as for having specified different clients in the first place.

I feel that this is not too important and more of an edge case but I would like to include it as the SDK also provides the functionality.

Billlynch commented 1 year ago

I like the proposal!

What do you think about adopting the React Suspense API for loading states?

toddbaert commented 1 year ago

One question that is open to me is how we want to handle multiple named clients in one "component".

@lukas-reining can you share a bit more about what might cause a consumer to need this? I can think of a few ways to handle it (your suggestion of an additional param to useFeatureFlags is a good one), but I'm also reluctant to complicate the initial version of an API to handle scenarios that aren't really needed.

I think since our SDKs currently support multiple providers, the react SDK should likely support the same thing, or we'll be "hiding" a featre. The use-case would be similar to the use-case in other SDKs - different sections of the app wanting to be associated with different backing feature-flag providers.

@moredip would you like to update the issue description with your proposals? I think you should have permission to do so.

toddbaert commented 1 year ago

I like the proposal!

What do you think about adopting the React Suspense API for loading states?

I'm not familiar with it, but I'll investigate it.

fahad19 commented 1 year ago

a half-finished PR lies in this repo here, showing how Suspense API can be utilized for feature flagging purposes in React: https://github.com/fahad19/featurevisor/pull/62

in case it helps.

Billlynch commented 1 year ago

I'd also like to think about how this could be used in a Next.js world. Ideally we would want to have the properties resolved on the server (in my opinion) and have that sent up with the page data. For this to work the data would need to be serialisable.

We could tackle this in it's own Next.js integration, where we can build an in-memory provider which is built of a serialisable object for example, or another, more flexible solution.

weyert commented 1 year ago

I think we should start with React 'vanilla' first and later work on popular framework integrations such as Next.js.

For example, your suggestion to fetch things on the server how do you have this imagined to work? Plenty of situations were we wouldn't know the targetKey on the server-side. Do you want to introduce some API route to exchange feature flag values? Currently, you can't query all enabled feature flags for targetKey.

moredip commented 1 year ago

I think since our SDKs currently support multiple providers, the react SDK should likely support the same thing, or we'll be "hiding" a featre. The use-case would be similar to the use-case in other SDKs - different sections of the app wanting to be associated with different backing feature-flag providers.

the PoC I have does support multiple providers (multiple named OF clients). You would achieve that by wrapping different parts of the app in different <OpenFeatureProvider> components.

What ISN'T support in the current PoC is using multiple named OF clients from within the same react component - you'd only be able to use the client from for whatever <OpenFeatureProvider> is in scope higher up the component heirarchy.

However, I'm a bit skeptical that needing using multiple OF clients from a single React component is going to come up very often at all. The main use case I see for multiple clients is the one @lukas-reining mentions - different teams using different providers. I don't see those multiple teams all having feature flags active within a specific react component very often at all.

We could certainly extend the useFeatureFlags API as @lukas-reining suggested, but I'd prefer to wait until it's a real proven use case.

moredip commented 1 year ago

I'd also like to think about how this could be used in a Next.js world. Ideally we would want to have the properties resolved on the server (in my opinion) and have that sent up with the page data. For this to work the data would need to be serialisable.

@Billlynch I'm not sure if it's possible to have an isomorphic hook. Wouldn't the server-rendered version need to be async, which implies it would need to use the server-side OF SDK.

I'm trying to remember if I've ever seen an isomorphic feature flag decision in the wild; I'm not sure if most vendor SDKs support that either, because of the sync/async API issue.

EDIT: hmm, this guidance on next.js from Launch Darkly is interesting reading.

lukas-reining commented 1 year ago

What ISN'T support in the current PoC is using multiple named OF clients from within the same react component - you'd only be able to use the client from for whatever is in scope higher up the component heirarchy.

@moredip Yes, this is exactly what I meant. I totally get that most times, different teams, with different providers would work on different components/pages etc. Still it could be that the providers should be high up in the hierarchy (above "pages") because teams want to have theire provider on all of theire pages. But in the end, I think not having It is not a blocker and It would be fine to wait and see if there is anyone needing it in reality 🙂

toddbaert commented 1 year ago

the PoC I have does support multiple providers (multiple named OF clients). You would achieve that by wrapping different parts of the app in different components.

@moredip Yes, this is exactly what I meant. I totally get that most times, different teams, with different providers would work on different components/pages etc. Still it could be that the providers should be high up in the hierarchy (above "pages") because teams want to have theire provider on all of theire pages. But in the end, I think not having It is not a blocker and It would be fine to wait and see if there is anyone needing it in reality slightly_smiling_face

Ah, OK, yes this seems sufficient to me. I agree that, at least for now, we can avoid supporting multiple providers in one component. I missed that technicality. As long as it's possible within one app, I think that's sufficient, and maybe even optimal in terms of keeping things simple for us.

TheMagoo73 commented 1 year ago

I think we should start with React 'vanilla' first and later work on popular framework integrations such as Next.js.

For example, your suggestion to fetch things on the server how do you have this imagined to work? Plenty of situations were we wouldn't know the targetKey on the server-side. Do you want to introduce some API route to exchange feature flag values? Currently, you can't query all enabled feature flags for targetKey.

I think this is probably a reference to React Server Components that are heavily used in the latest versions of Next with the App Router, but equally are now ‘vanilla React’. They work well with Suspense, and are ideal for remote data fetching. I’d certainly second the proposal to add support for them as they would be a requirement for my projects to adopt an SDK.

toddbaert commented 1 year ago

I've opened https://github.com/open-feature/js-sdk/pull/546, which mostly just lays the repo groundwork, but also has a basic implementation (NOT FINAL) by @moredip.

I've also transferred this issue to the js-sdk (out of contrib).

juanparadox commented 10 months ago

Have we given any thought to adding some kind of <OpenFeature>{some child}</OpenFeature> component that takes in a feature flag boolean result and based on that result conditionally renders the children?

A possible benefit of a component that renders some thing based on a feature flag is that it can Suspend the render of the children until two conditions are met - the provider event is set to ready and the feature is true.

I also understand if this is overkill:

toddbaert commented 10 months ago

Have we given any thought to adding some kind of <OpenFeature>{some child}</OpenFeature> component that takes in a feature flag boolean result and based on that result conditionally renders the children?

A possible benefit of a component that renders some thing based on a feature flag is that it can Suspend the render of the children until two conditions are met - the provider event is set to ready and the feature is true.

I also understand if this is overkill:

  • If we should expect the 'readiness' of the consuming components to have only rendered if the status of the set provider is ready then this is a redundancy
  • If using the useFeatureFlag hook is enough then this is definitely a redundancy

@juanparadox

That's an interesting idea... it sounds like a combination of useStatus hook I mentioned above with a useFlag boolean hook. So it seems like "syntactic sugar", but possibly worth adding. I think it'd have to support an arg for a provider name too. We'd need to work on a better name though as well: maybe ShowFeature or IsEnabled?

moredip commented 10 months ago

Yeah it's an interesting idea - a lot of React codebases end up with a hand-rolled version of this.

We'd probably also want to support an A/B type thing, where you specify what the on and off components should be. The Split documentation has an example of a basic "toggler" component that does this: https://help.split.io/hc/en-us/articles/360038825091-React-SDK#get-treatments-with-configurations

toddbaert commented 4 months ago

Closing with first non-experimental release.