preactjs / signals

Manage state with style in every framework
https://preactjs.com/blog/introducing-signals/
MIT License
3.71k stars 91 forks source link

`@preact/signals-react` 1.3.6 is brokes useSyncExternalStoreWithSelector #411

Open XantreDev opened 12 months ago

XantreDev commented 12 months ago

I am using @tanstack/react-router which using useSyncExternalStoreWithSelector i've worked with this combo for week, but yesterday it started to throw inside react internals. I has really long research and fixed this issue by downgrading to 1.2.2 where was no react internals patching. I dont really know how to reproduce this issue, maybe it's because signals uses sync external store too

react-dom.development.js:16249 Uncaught TypeError: Cannot read properties of undefined (reading 'length')
    at areHookInputsEqual (react-dom.development.js:16249:38)
    at updateMemo (react-dom.development.js:17240:11)
    at Object.useMemo (react-dom.development.js:17886:16)
    at useMemo (react.development.js:1650:21)
    at useSyncExternalStoreWithSelector (with-selector.development.js:64:18)
    at useStore (index.tsx:17:17)
    at useRouterState (react.tsx:568:10)
    at useMatch (react.tsx:673:24)
    at useSearch (react.tsx:802:10)
    at _Task (Task.tsx:23:57)
areHookInputsEqual @ react-dom.development.js:16249
updateMemo @ react-dom.development.js:17240
useMemo @ react-dom.development.js:17886
useMemo @ react.development.js:1650
useSyncExternalStoreWithSelector @ with-selector.development.js:64
useStore @ index.tsx:17
useRouterState @ react.tsx:568
useMatch @ react.tsx:673
useSearch @ react.tsx:802
_Task @ Task.tsx:23
renderWithHooks @ react-dom.development.js:16305
updateFunctionComponent @ react-dom.development.js:19588
updateSimpleMemoComponent @ react-dom.development.js:19425
beginWork @ react-dom.development.js:21678
callCallback2 @ react-dom.development.js:4164
invokeGuardedCallbackDev @ react-dom.development.js:4213
invokeGuardedCallback @ react-dom.development.js:4277
beginWork$1 @ react-dom.development.js:27451
performUnitOfWork @ react-dom.development.js:26557
workLoopSync @ react-dom.development.js:26466
renderRootSync @ react-dom.development.js:26434
performSyncWorkOnRoot @ react-dom.development.js:26085
flushSyncCallbacks @ react-dom.development.js:12042
(anonymous) @ react-dom.development.js:25651
setTimeout (async)
(anonymous) @ utils.ts:405
sleep @ utils.ts:404
scheduleMicrotask @ utils.ts:414
flush @ notifyManager.ts:64
batch @ notifyManager.ts:31
dispatch @ query.ts:590
setData @ query.ts:204
onSuccess @ query.ts:472
resolve @ retryer.ts:103
Promise.then (async)
run @ retryer.ts:153
createRetryer @ retryer.ts:204
fetch @ query.ts:458
executeFetch @ queryObserver.ts:350
onSubscribe @ queryObserver.ts:107
subscribe @ subscribable.ts:15
(anonymous) @ useBaseQuery.ts:81
subscribeToStore @ react-dom.development.js:16958
commitHookEffectListMount @ react-dom.development.js:23150
commitPassiveMountOnFiber @ react-dom.development.js:24926
commitPassiveMountEffects_complete @ react-dom.development.js:24891
commitPassiveMountEffects_begin @ react-dom.development.js:24878
commitPassiveMountEffects @ react-dom.development.js:24866
flushPassiveEffectsImpl @ react-dom.development.js:27039
flushPassiveEffects @ react-dom.development.js:26984
commitRootImpl @ react-dom.development.js:26935
commitRoot @ react-dom.development.js:26682
performSyncWorkOnRoot @ react-dom.development.js:26117
flushSyncCallbacks @ react-dom.development.js:12042
(anonymous) @ react-dom.development.js:25651
Show 52 more frames
Show less
react-dom.development.js:16249 Uncaught TypeError: Cannot read properties of undefined (reading 'length')
    at areHookInputsEqual (react-dom.development.js:16249:38)
    at updateMemo (react-dom.development.js:17240:11)
    at Object.useMemo (react-dom.development.js:17886:16)
    at useMemo (react.development.js:1650:21)
    at useSyncExternalStoreWithSelector (with-selector.development.js:64:18)
    at useStore (index.tsx:17:17)
    at useRouterState (react.tsx:568:10)
    at useMatch (react.tsx:673:24)
    at useSearch (react.tsx:802:10)
    at _Task (Task.tsx:23:57)
diego9497 commented 11 months ago

I'm having the same issue, I'm using @preact/signals-react@1.3.4, @reduxjs/toolkit"@1.8.5 and react@17.0.2, downgrade doesn't work for me since start thrown errors with the router.

react-dom.development.js:15866 Uncaught Error: TypeError: Cannot read properties of undefined (reading 'length')
    at updateMemo (react-dom.development.js:15866:1)
    at Object.useMemo (react-dom.development.js:16422:1)
    at useMemo (react.development.js:1532:1)
    at useSyncExternalStoreWithSelector (with-selector.development.js:64:1)
    at useSelector (useSelector.js:41:1)
    at TaskSubtasksSection (task-subtasks-section.tsx:34:38)
    at renderWithHooks (react-dom.development.js:14985:1)
    at updateFunctionComponent (react-dom.development.js:17365:1)
    at beginWork (react-dom.development.js:19072:1)
    at HTMLUnknownElement.callCallback (react-dom.development.js:3945:1)
XantreDev commented 11 months ago

@diego9497 can you provide repro? Is it enough to install signals, redux toolkit and use it just once?

lucasbarrosomk6 commented 10 months ago

Ran into this problem as well. "@preact/signals-react": "1.3.6", doesn't play well with redux toolkit, is there a solution in the works?

lucasbarrosomk6 commented 10 months ago

It is enough to install signals in a redux toolkit project and use it just once.

jholmes033169 commented 10 months ago

Also getting this issue, also using redux (but not not toolkit). Downgrading did solve the problem, but I don't want to be stuck on 1.2.2. Attempting to upgrade to latest version of redux and see how this affects things.

XantreDev commented 10 months ago

Do you need to do something especial in rudux to have this issue. If you will provide it I will try to create test case

XantreDev commented 10 months ago

I've tried to reproduce it by writing simple counter, but all is working fine. What do I need to do to reproduce this behaviour? https://github.com/XantreGodlike/preact-signals-redux-issue

jholmes033169 commented 9 months ago

Sorry... li will try to set up a test.

On Sun, Nov 12, 2023, 12:47 AM Valerii Smirnov @.***> wrote:

I've tried to reproduce it by writing simple counter, but all is working fine. What do I need to do to reproduce this behaviour? https://github.com/XantreGodlike/preact-signals-redux-issue

— Reply to this email directly, view it on GitHub https://github.com/preactjs/signals/issues/411#issuecomment-1807012228, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZDYABBSD2WKLHLH5HAXXTYEBPHPAVCNFSM6AAAAAA4SJNTZKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBXGAYTEMRSHA . You are receiving this because you commented.Message ID: @.***>

jholmes033169 commented 9 months ago

Ok... so the only thing I need to do to reproduce this is add and setup react-redux in the a project. Straight up react-redux will do it... doesn't even need to be toolkit. I'm using react-redux ^8.0.2, react": "^18.2.0 and "redux": "^4.0.4",

XantreDev commented 9 months ago

Seems to be this issue related with react patching. Some default behaviour of react breaks after monkeypatching and this issue appears in big projects

jholmes033169 commented 9 months ago

Yeah... confirmed it with another project with react-redux. I just pulled signals-react in and created a signal and had the same issue. This is a huge problem for migrating. We had hoped to move our thinking gradually from redux to signals. I don't know what the repercussions of sticking with 1.2.2 until we can fully migrate are, though.

batlash commented 9 months ago

Hi, I had the same problem. I had the following problem/warning (before I added the signal), which in my case seems to be the root case; Warning: Each child in a list should have a unique "key" prop. When that warning was resolved, this problem with the signal also was resolved (which caused that the application could not render). I hope that this could help.

XantreDev commented 9 months ago

Warning: Each child in a list should have a unique "key" prop.

So you've had elements with the same key and it caused this error, isn't it?

Cannot read properties of undefined (reading 'length')
batlash commented 9 months ago

Exactly, (or rather I had no key at all). So the solution in my case was to add a key (in a component that had noting to do with the signal).

(<div className="container">
                    {movies.map((movie) => <MovieCard  key={movie.imdbID} movie={movie} onClick={onCardClicked} />)}
 </div>)
andrewiggins commented 9 months ago

Our next major version of signals-react (hopefully coming out in the next couple weeks) is gonna deprecate and make opt-in the current React internal patching in favor of using a Babel transform to make components reactive to signals.

The transform is available now on NPM for trying out but some internal changes/fixes & more/updated documentation are needed before I consider it stable. I suspect that the transform won't have similar issues.

XantreDev commented 9 months ago

@andrewiggins I don't think this approach scales well - because we will lack support of next.js. Since their babel integration is kinda legacy and not all features will work with babel (next-fonts doesn't). So seems to be we cannot rely on transform, probably there should be transform for other parsers (swc, esbuild). For now I don't want to increate complexity and prepared manual intergation HOC for such environments, check my integration. I would like you to give me some feedback (it based on transform and signal-react integration source code)