Skip to content

Add forceScope parameter to useUnit in react #776

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

Merged

Conversation

sagdeev
Copy link
Contributor

@sagdeev sagdeev commented Sep 5, 2022

Conventions

Resolves #765

@netlify
Copy link

netlify bot commented Sep 5, 2022

👷 Deploy request for effector-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 18f74d2

@sagdeev sagdeev marked this pull request as ready for review September 5, 2022 11:07
Copy link
Member

@igorkamyshev igorkamyshev left a comment

Choose a reason for hiding this comment

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

Sounds great to me 💙

Folks, @effector/core what do you think?

Copy link
Member

@AlexandrHoroshih AlexandrHoroshih left a comment

Choose a reason for hiding this comment

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

Hi @sagdeev !
Thanks for contribution!

Everything looks good, but there is a bit of misunderstanding, probably:

In the original issue useUnit without forceScope must try to find the scope from context and if scope is not found - work without it

So overall there a few things missing:

  1. Last test snapshot with forceScope: false shoud have a value from scope
  2. Need to add another test with forceScope: false and without Provider (should work like it currently works)
  3. Need to add isomorphic behaviour


expect(container.firstChild).toMatchInlineSnapshot(`
<div>
value
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, looks like there is a bit of misunderstanding 🤔

In the original issue #765 there is a requirement for an isomorphic behavior, which basically means useUnit without forceScope will try to get scope from context and if its not found -> work like there is no scope

So this test should be updated this way:

Suggested change
value
scoped value

export function useUnit(shape) {
return useUnitBase(shape)
export function useUnit(shape, opts?: {forceScope?: boolean}) {
return useUnitBase(shape, opts?.forceScope ? getScope() : undefined)
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to add isomorphic behaviour here something like this:

Suggested change
return useUnitBase(shape, opts?.forceScope ? getScope() : undefined)
return useUnitBase(shape, getScope(opts?.forceScope))

src/react/ssr.ts Outdated
Comment on lines 21 to 23
export function getScope() {
const scope = React.useContext(ScopeContext)
if (!scope)
Copy link
Member

Choose a reason for hiding this comment

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

It looks like a simplest place to introduce isomorphic behavoir to me, but maybe there a better ones 🤔

I think, it may look like this:

Suggested change
export function getScope() {
const scope = React.useContext(ScopeContext)
if (!scope)
export function getScope(forceScope?: boolean) {
const scope = React.useContext(ScopeContext)
if (forceScope && !scope)

Copy link
Member

@sergeysova sergeysova Sep 5, 2022

Choose a reason for hiding this comment

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

I think we need to move getScope out from the ssr.ts file.
Because all export from this module is available to the end-user.

@sagdeev
Copy link
Contributor Author

sagdeev commented Sep 9, 2022

@AlexandrHoroshih hi! I think I understand what you were talking about 😃 I made some changes as you requested. Can you please take a look?

  • now it works as expected
  • update tests
  • move getScope to new file and add exports

Copy link
Member

@AlexandrHoroshih AlexandrHoroshih left a comment

Choose a reason for hiding this comment

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

@sagdeev Awesome, looks perfect 🔥👍

@AlexandrHoroshih AlexandrHoroshih merged commit f3c9aaa into effector:master Sep 9, 2022
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.

Isomorphic useUnit in effector-react
4 participants