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: use mutate context hook #1031

Merged
merged 13 commits into from
Oct 17, 2024
Merged

Conversation

wichopy
Copy link
Member

@wichopy wichopy commented Oct 6, 2024

This PR

New hook for mutating context instead of using the global object

Related Issues

Fixes #968

Notes

Follow-up Tasks

How to test

@wichopy wichopy requested a review from a team as a code owner October 6, 2024 18:16
@beeme1mr
Copy link
Member

beeme1mr commented Oct 7, 2024

Could you please sign off on your commits? It's basically a way to tell the CNCF that you're willing and able to donate this code.

To add your Signed-off-by line to every commit in this branch:

For future commits, you can add -s when running git commit.

I'll look into the licensing issue.

Signed-off-by: Will Chou <[email protected]>
Signed-off-by: Will Chou <[email protected]>
Signed-off-by: Will Chou <[email protected]>
Signed-off-by: Will Chou <[email protected]>
@@ -136,5 +139,74 @@ describe('OpenFeatureProvider', () => {
await waitFor(() => expect(screen.queryByText('👍')).toBeInTheDocument(), { timeout: DELAY * 2 });
});
});

describe('useMutateContext', () => {
Copy link
Member

Choose a reason for hiding this comment

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

These tests look great but could you please add one more for the following setup?

<OpenFeatureProvider domain={DOMAIN1}>
  <React.Suspense fallback={<div>{FALLBACK}</div>}>
    <TestComponent name="Will" />
    <OpenFeatureProvider>
      <React.Suspense fallback={<div>{FALLBACK}</div>}>
        <TestComponent name="Todd" />
      </React.Suspense>
    </OpenFeatureProvider>
  </React.Suspense>
</OpenFeatureProvider>

The nested component should update the global context.

Copy link
Member Author

@wichopy wichopy Oct 11, 2024

Choose a reason for hiding this comment

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

@beeme1mr I have set this up. In this scenario, it will update both <TestComponents />. Is that the expected behaviour?

Copy link
Member

Choose a reason for hiding this comment

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

I'd like a test that ensures that updating context in the nested component updates the global context and not the domain scoped context. The other component probably isn't necessary in the test.

Copy link
Member Author

@wichopy wichopy Oct 12, 2024

Choose a reason for hiding this comment

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

The reason I'm asking is it seems like something is wrong, shouldn't domain scoped children be unchanged even if the global context changes?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you're correct. Is there something wrong with my example?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

@wichopy wichopy Oct 16, 2024

Choose a reason for hiding this comment

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

@toddbaert adding a property to set global or not doesn't solve the issue I brought up though.

I've added a test with the global context having a different flag key and variants than the domain scoped one. The domain and global provider context evaluators both listen to user === '[email protected]', but now only the global context flag variant changes instead of both global and domain scoped.

When the providers are the same (same flag keys and variants) they both update when global context is nested under domain context. Did I find a bug or is this desired behaviour?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure I understand your last comment. But I can confirm what @beeme1mr said here:

Basically, domain-scoped context is only managed separately if it's explicitly set for the domain.

Basically, if you don't set a context for domain X, it will use the default context. If you DO set a context for domain X, it uses that context (this is analogous to how providers are bound to domains as well. Does that clear things up?

My point about the option was somewhat separate: just that users might want to update either of these from their component, and either is valid.

Copy link
Member Author

Choose a reason for hiding this comment

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

i think you cleared it up! i think testing is all set then unless we need more cases.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, looks that way to me.

packages/react/src/provider/use-context-mutator.ts 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.

Looks good, thank you! :)
I left one comment to be considered.

packages/react/src/provider/use-context-mutator.ts Outdated Show resolved Hide resolved
wichopy and others added 2 commits October 12, 2024 15:21
Signed-off-by: Will Chou <[email protected]>
Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

LGTM @wichopy ! Thanks so much! I might make some additional small doc changes before we release, and we have some other features soon ready so I'll wait until they are all done, but this will be released soon.

@toddbaert toddbaert merged commit ec3d967 into open-feature:main Oct 17, 2024
6 of 8 checks passed
toddbaert added a commit that referenced this pull request Oct 17, 2024
Minor refactors, but mostly adding js-doc after
#1031.

---------

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.

[FEATURE] React SDK: Provide hook to set relevant context "useContextMutator"
4 participants