facebookarchive / redux-react-hook

React Hook for accessing state and dispatch from a Redux store
MIT License
2.16k stars 103 forks source link

use useEffect instead of useLayoutEffect #55

Closed istarkov closed 5 years ago

istarkov commented 5 years ago

Setting here https://github.com/facebookincubator/redux-react-hook/blob/e8830371ec22fe171a98f1b537a7a3f8e8ef0af3/src/create.ts#L115 useEffect instead of useIsomorphic... gives zero failing tests, even test provided for useLayoutEffect change works fine. From what I see there is no real need of using useLayoutEffect in that place or any real test needed.

ianobermiller commented 5 years ago

@goloveychuk Can you please take a look? It seems like the tests would have passed without the product changes.

goloveychuk commented 5 years ago

I have

● redux-react-hook › useMappedState › renders once if have multiple useMappedState

    expect(received).toBe(expected) // Object.is equality

    Expected: 3
    Received: 5

      286 |       updateStoreWithoutAct({bar: 12, foo: '12'});
      287 |       act(() => {});
    > 288 |       expect(renderCount).toBe(3);
          |                           ^
      289 |     });
      290 |
      291 |     it('throws if provider is missing', () => {

      at Object.toBe (src/__tests__/index-test.tsx:288:27)

when using useEffect

istarkov commented 5 years ago

Try clean install as I have zero errors on master image with useEffect(() => { setted here https://github.com/facebookincubator/redux-react-hook/blob/master/src/create.ts#L115

Just console errors Warning: An update to Component inside a test was not wrapped in act(...).

istarkov commented 5 years ago

PS: Your error is only shown if both effects changed on useEffect, but only last must be changed

goloveychuk commented 5 years ago

Yea, we can use “useEffect” in subscribe function.

ianobermiller commented 5 years ago

@istarkov Would you like to send a PR?

ianobermiller commented 5 years ago

@goloveychuk are you up for fixing this?

ianobermiller commented 5 years ago

So I wrote some tests that don't work if you useEffect, so I think it is wise to keep useLayoutEffect for now.