reduxjs / react-redux

Official React bindings for Redux
https://react-redux.js.org
MIT License
23.38k stars 3.37k forks source link

TypeError: Cannot redefine property: useSelector #1920

Closed osliver closed 1 year ago

osliver commented 2 years ago

What version of React, ReactDOM/React Native, Redux, and React Redux are you using?

What is the current behavior?

In some test cases I used spyOn for mock useSelector hook -

import * as ReactRedux from 'react-redux';
jest.spyOn(ReactRedux, 'useSelector').mockReturnValueOnce({value: mockValue});

after updating to "8.0.1", I'm getting error -

TypeError: Cannot redefine property: useSelector
        at Function.defineProperty (<anonymous>)

What is the expected behavior?

everything was okay on

"react-redux": "7.2.8",
"redux": "4.1.2",

Which browser and OS are affected by this issue?

No response

Did this work in previous versions of React Redux?

markerikson commented 2 years ago

I don't have a particular answer for this, tbh. We are still using Babel for the build output, just doing TS transpilation as part of that. I did turn off some of the IE11 compat settings along the way, but we should still have CJS and ESM build artifacts.

Having said that, we would strongly recommend against trying to mock the hooks in tests anyway! Instead, we specifically recommend writing "integration"-style tests that create a real Redux store with test data, and rendering a <Provider> around your components - not mocking the hooks.

See https://redux.js.org/usage/writing-tests for our recommended approach to testing.

Given that, I'm going to close this.

If anyone else does want to look into what the technical issue is here, please feel free to investigate and contribute, but this does not seem like something that we're going to spend time working on ourselves.

pjg commented 2 years ago

I've also run into this issue. The above method for mocking out useSelector or useDispatch no longer works, and with upgrading to latest react-redux all tests written in this way fail without a clear and/or gradual way to move forward. The only option seems to be to switch to Redux Toolkit and the test setup for this, but that requires a lot of effort.

See how this method is recommended in various places:

markerikson commented 2 years ago

For the record this has nothing to do with Redux Toolkit, at all.

If anything, it's something about how we updated our build steps in React-Redux as part of the TS conversion process in React-Redux v8. (I'll also point out that we did that conversion last summer, and had alphas and betas out for months, but we got very little feedback on any of those pre-releases... and definitely nothing regarding this issue.)

But regardless of that, the suggestion to "mock the network layer, not our hooks" is still fully applicable no matter how the actual Redux logic is written. It should work just as well with legacy "hand-written" Redux logic and thunks as it would with RTK-based code.

I'll also point out that we, the maintainers, have never recommended mocking the hooks, ever, in any of our documentation. That's something folks in the community decided to try doing.

I'm open to suggestions of possible tweaks to the build process that might help, if there are any... but in general we do not think mocking React-Redux hooks is a good idea in the first place!.

pjg commented 2 years ago

I understand, and I agree that mocking hooks is probably not the best way to go about things. However, this is a very simple "api" in that all it takes is 3 lines of code and I have react-redux stare mocked in my test(s). I cannot find an alternative for my use-case (i.e. not using RTK), when testing components using redux (via useDispatch / useSelector). A hint/guide/link as to how to proceed in our case would be extremely helpful.

markerikson commented 2 years ago

@pjg that's part of the point of our recommendations at https://redux.js.org/usage/writing-tests#components . Your test code will not care how the logic has been written. It will not matter whether it's a single component with just React state, or a huge complex tree of Redux connected components dispatching thunks or using RTK Query. If you set up a Redux store, wrap the component under test in a <Provider store={store}>, mock out the network request via msw or mirage, and click "Fetch Some Data", the Redux logic will kick in and run, regardless of how that logic is implemented.

markerikson commented 2 years ago

FYI, @phryneas just did a bit of investigation, and it looks like the actual exports definitions may be fixable if we tweak our Babel settings:

Doesn't change our recommendation about not mocking hooks, but if changing that setting improves the build output in a way that is more compatible with Jest mocking, I'm up for releasing it.

phryneas commented 2 years ago

I will add to that that this is caused by a babel default setting. Mocking imports is extremely flimsy and cannot really be relied on. While we can make this work for now, I want to 💯 stress what @markerikson already said a few times: this is not how we recommend testing components. Testing components in isolation with directly mocked methods will not give you a lot of confidence at all - you are essentially testing implementation details, not actual functionality.

cafesanu commented 2 years ago

This solved our issue:

const mockDispatch = jest.fn();
const mockSelector = jest.fn();

jest.mock("react-redux", () => ({
 ...jest.requireActual("react-redux"),
 useDispatch: () => mockDispatch,
 useSelector: () => mockSelector,
}));
markerikson commented 2 years ago

@cafesanu This should have been fixed as of 8.0.2.

Note that we still specifically advise against doing this form of testing at all! But it should at least not crash.

arcollector commented 2 years ago

is this fixed? i am using 8.0.2 getting this error still

markerikson commented 2 years ago

@arcollector it should have been fixed. Can you post a project that shows this still happening?

waaaaaitaminute. I take that back. Did we not actually make this change?

Maybe I mixed this up with something else, because I'm looking at the 8.0.2 release notes and changes and I don't see this.

markerikson commented 2 years ago

Yeah. Sorry, I was wrong when I closed this.

Lenz pointed out that the setting could be changed. But no one has filed a PR to actually change this setting.

markerikson commented 1 year ago

Looking back at this again.

It's still not a practice we want users to do, and it looks like there's a userland workaround at https://github.com/reduxjs/react-redux/issues/1920#issuecomment-1147559047 .

Given that, I'm going to close this for real this time.

jamesryan-dev commented 1 year ago

@cafesanu could you provide an example of your solution in a test please? I'm struggling to get your example working

sushrut-walmart commented 10 months ago

This solved our issue:

const mockDispatch = jest.fn();
const mockSelector = jest.fn();

jest.mock("react-redux", () => ({
 ...jest.requireActual("react-redux"),
 useDispatch: () => mockDispatch,
 useSelector: () => mockSelector,
}));

I think there is a syntax error. Spent hours trying to figure out why it was not working. Needed paranthesis after mockSelector

const mockDispatch = jest.fn();
const mockSelector = jest.fn();

jest.mock("react-redux", () => ({
 ...jest.requireActual("react-redux"),
 useDispatch: () => mockDispatch,
 useSelector: () => mockSelector(),
}));