Add forceScope parameter to useUnit in react #776
Conversation
👷 Deploy request for effector-docs pending review.Visit the deploys page to approve it
|
igorkamyshev
left a comment
There was a problem hiding this comment.
Sounds great to me 💙
Folks, @effector/core what do you think?
There was a problem hiding this comment.
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:
- Last test snapshot with
forceScope: falseshoud have a value from scope - Need to add another test with
forceScope: falseand without Provider (should work like it currently works) - Need to add isomorphic behaviour
|
|
||
| expect(container.firstChild).toMatchInlineSnapshot(` | ||
| <div> | ||
| value |
There was a problem hiding this comment.
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:
| value | |
| scoped value |
src/react/nossr.ts
Outdated
| export function useUnit(shape) { | ||
| return useUnitBase(shape) | ||
| export function useUnit(shape, opts?: {forceScope?: boolean}) { | ||
| return useUnitBase(shape, opts?.forceScope ? getScope() : undefined) |
There was a problem hiding this comment.
I suggest to add isomorphic behaviour here something like this:
| return useUnitBase(shape, opts?.forceScope ? getScope() : undefined) | |
| return useUnitBase(shape, getScope(opts?.forceScope)) |
src/react/ssr.ts
Outdated
| export function getScope() { | ||
| const scope = React.useContext(ScopeContext) | ||
| if (!scope) |
There was a problem hiding this comment.
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:
| export function getScope() { | |
| const scope = React.useContext(ScopeContext) | |
| if (!scope) | |
| export function getScope(forceScope?: boolean) { | |
| const scope = React.useContext(ScopeContext) | |
| if (forceScope && !scope) |
There was a problem hiding this comment.
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.
|
@AlexandrHoroshih hi! I think I understand what you were talking about 😃 I made some changes as you requested. Can you please take a look?
|
AlexandrHoroshih
left a comment
There was a problem hiding this comment.
@sagdeev Awesome, looks perfect 🔥👍
Conventions
Resolves #765