Add forceScope opts to useUnit in solidjs #782
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.
Thanks for a contribution 💙
src/solid/scope.ts
Outdated
| export const {Provider} = ScopeContext | ||
|
|
||
| function getScope() { | ||
| export function getScope(forceScope?: boolean) { |
There was a problem hiding this comment.
In my opinion, it is better not to export this function from public module, let's move it somewhere inside the package?
There was a problem hiding this comment.
Maybe take move everything the scope.ts into an ssr.ts. Leave only public methods in the ssr.ts.
There was a problem hiding this comment.
Yep, that would be better move 🤔
getScope should not be public, that's for sure
e3e495c to
4233ac9
Compare
AlexandrHoroshih
left a comment
There was a problem hiding this comment.
Thank you for your contribution! 🧡
Everything looks good, all test in place, feature works as expected 🔥 👍
There only few small caveats:
src/solid/scope.ts
Outdated
| export function useUnit(shape) { | ||
| return useUnitBase(shape, getScope()) | ||
| export function useUnit(shape, opts?: {forceScope?: boolean}) { | ||
| const scope = getScope(opts?.forceScope) |
There was a problem hiding this comment.
I think that /scope version of bindings should have forceScope enabled by default 🤔
I suggest to change it like that:
| const scope = getScope(opts?.forceScope) | |
| const scope = getScope(opts?.forceScope ?? true) |
There was a problem hiding this comment.
Hmm, i thougth a little more and now i suggest to do it like this:
-
Inside
getScopethere should be default fallback likeforceScope = true -
For
/scopeexports we don't need to do anything besidesgetScope()call without params -
For main
effector-solidexports we should do something likegetScope(ops?.forceScope ?? false)inside
src/solid/scope.ts
Outdated
| export const {Provider} = ScopeContext | ||
|
|
||
| function getScope() { | ||
| export function getScope(forceScope?: boolean) { |
There was a problem hiding this comment.
Yep, that would be better move 🤔
getScope should not be public, that's for sure
|
I don't pass the |
|
Hmm, @Drevoed maybe you can help? 🤔 I don't quite understand, what is happening in the tests and why changes in PR would affect this |
|
@usmanyunusov nevermind, i got it. You moved hooks into I suggest to revert these changes (get hooks back into |
|
There is one in React bindings, but it is a leftover of past times, when there were no other usecases for fork-api, than ssr Now it is deprecated, basically, as In solid.js bindings it was done this way from the start |
ca4356c to
5343de4
Compare
|
@AlexandrHoroshih, Fixed |
AlexandrHoroshih
left a comment
There was a problem hiding this comment.
Looks good!
@Drevoed what you think?
|
LGTM, awesome work! Thanks :) 🚀 |
Conventions
Fix #766