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

feat: suspense support, client scoping, context-sensitive re-rendering #698

Merged
merged 10 commits into from
Jan 11, 2024

Conversation

toddbaert
Copy link
Member

@toddbaert toddbaert commented Nov 29, 2023

Adds a few react features:

  • <Suspense /> support: components using feature flags will trigger suspense for easy loaders/spinners
  • Ability to specify name for provider scope: <OpenFeatureProvider name="my-provider">

Important

Please see here for the latest changes to the demo application using these features (note that we won't be able to merge these demos until this is released).

Important

Also note I've added no tests, which is certainly not my MO. I will add them to this PR once there's an agreement on this behavior and implementation.

gif from the demo app:

demo

⚠️ I want to add another feature here to support re-rendering on context changes. That requires this to be merged.

@beeme1mr
Copy link
Member

I know React uses the term Provider but we may want to come up with an alternative since a provider already has a specific meaning in OpenFeature.

image

@toddbaert
Copy link
Member Author

toddbaert commented Nov 29, 2023

I know React uses the term Provider but we may want to come up with an alternative since a provider already has a specific meaning in OpenFeature.

@beeme1mr I think I disagree. Conveniently the React provider is easily associated with a OpenFeature Provider via the name... so in some respects it actually works well to blur these lines. Moreover, in React, providers are a very explicit thing, and if we don't use that name I think people would be confused.

Maybe somebody else has an opinion on this point specifically. cc @lukas-reining @thomaspoignant @kinyoklion

@weyert
Copy link
Contributor

weyert commented Nov 29, 2023

I know React uses the term Provider but we may want to come up with an alternative since a provider already has a specific meaning in OpenFeature.

@beeme1mr I think I disagree. Conveniently the React provider is easily associated with a OpenFeature Provider via the name... so in some respects it actually works well to blur these lines. Moreover, in React, providers are a very explicit thing, and if we don't use that name I think people would be confused.

Maybe somebody else has an opinion on this point specifically. cc @lukas-reining @thomaspoignant @kinyoklion

No strong feelings about it but you could always call it OpenFeatureContextProvider

@toddbaert
Copy link
Member Author

I know React uses the term Provider but we may want to come up with an alternative since a provider already has a specific meaning in OpenFeature.

@beeme1mr I think I disagree. Conveniently the React provider is easily associated with a OpenFeature Provider via the name... so in some respects it actually works well to blur these lines. Moreover, in React, providers are a very explicit thing, and if we don't use that name I think people would be confused.
Maybe somebody else has an opinion on this point specifically. cc @lukas-reining @thomaspoignant @kinyoklion

No strong feelings about it but you could always call it OpenFeatureContextProvider

Ya, I'd be fine with that, but unfortunately we also use Context in OpenFeature 😬

@lukas-reining
Copy link
Member

lukas-reining commented Nov 29, 2023

No strong feelings about it but you could always call it OpenFeatureContextProvider.

Also no strong feelings.
OpenFeatureProvider feels the best to me. For people working on React it seems to be the most intuitive and as it is only used in the JSX context, I also feel that people won't confuse this for the OpenFeature provider concept.

@beeme1mr
Copy link
Member

You've convinced me. I'm fine with OpenFeatureProvider.

@thomaspoignant
Copy link
Member

I agree with @lukas-reining OpenFeatureProvider is simple enough.

packages/client/src/client/open-feature-client.ts Outdated Show resolved Hide resolved
packages/react/src/provider.tsx Outdated Show resolved Hide resolved
Copy link
Member

@lukas-reining lukas-reining left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good to me and is a nice addition.
Left two comments with open questions.

packages/react/src/provider.tsx Outdated Show resolved Hide resolved
packages/react/src/use-feature-flag.ts Outdated Show resolved Hide resolved
github-merge-queue bot pushed a commit that referenced this pull request Dec 1, 2023
This is separate PR for a similar change
[here](#698).

I'm hesitant to spec this at the moment. I'm only implementing it in the
client because it seems particularly useful for React.

Signed-off-by: Todd Baert <[email protected]>
Copy link
Contributor

@weyert weyert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jonathannorris
Copy link
Member

@toddbaert / @beeme1mr one question we had taking a look at this and thinking through the suspense behaviour, what happens when the context is updated? does the SDK go back into a "pending" state?

@toddbaert
Copy link
Member Author

@toddbaert / @beeme1mr one question we had taking a look at this and thinking through the suspense behaviour, what happens when the context is updated? does the SDK go back into a "pending" state?

That's a very interesting question...

When the context is set, the SDK doesn't change the provider state (and has no such state itself) - but some providers might go into the ProviderStatus.STALE state while they await new flag values for the updated context. Others (particularly those that cache rulesets) may not.

This has a few implications, and I think we are missing some guidance and possibly some semantics around the on context changed handler and provider state/events. I will think about this some more today, and possibly create an issue and a PR in spec. Thanks for the thoughtful consideration @jonathannorris !

Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
@toddbaert toddbaert marked this pull request as ready for review January 11, 2024 18:49
Signed-off-by: Todd Baert <[email protected]>
@toddbaert
Copy link
Member Author

@toddbaert / @beeme1mr one question we had taking a look at this and thinking through the suspense behaviour, what happens when the context is updated? does the SDK go back into a "pending" state?

That's a very interesting question...

When the context is set, the SDK doesn't change the provider state (and has no such state itself) - but some providers might go into the ProviderStatus.STALE state while they await new flag values for the updated context. Others (particularly those that cache rulesets) may not.

This has a few implications, and I think we are missing some guidance and possibly some semantics around the on context changed handler and provider state/events. I will think about this some more today, and possibly create an issue and a PR in spec. Thanks for the thoughtful consideration @jonathannorris !

OK, so I've implemented another feature which allows for re-renders with the emission of ProviderEvents.ContextChanged events. This means that once the context is reconciled, we can re-render components, which is a very important thing to have. Currently, there's no "pending" state; the aforementioned event is only emitted once the provider has fully reconciled it's new context (or it fails to, in which case an error is emitted).

packages/react/README.md Outdated Show resolved Hide resolved
packages/react/src/provider.tsx Outdated Show resolved Hide resolved
toddbaert and others added 3 commits January 11, 2024 14:13
Co-authored-by: Michael Beemer <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
@toddbaert toddbaert changed the title feat: react suspense support, named clients feat: suspense support, client scoping, context-sensitive re-rendering Jan 11, 2024
@toddbaert toddbaert added this pull request to the merge queue Jan 11, 2024
Merged via the queue into main with commit 68b0f26 Jan 11, 2024
9 checks passed
@toddbaert toddbaert deleted the feat/suspense branch January 11, 2024 20:12
@toddbaert toddbaert restored the feat/suspense branch January 11, 2024 20:16
toddbaert added a commit that referenced this pull request Jan 11, 2024
#759)

Adds a few react features:

- `<Suspense />` support: components using feature flags will trigger
suspense for easy loaders/spinners
- Ability to specify name for provider scope: `<OpenFeatureProvider
name="my-provider">`
- re-render on context change

This is an exact re-merge of:
#698, without a web-sdk
whitespace change that created a bunch of bad release notes.

Signed-off-by: Todd Baert <[email protected]>
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

Successfully merging this pull request may close these issues.

7 participants