reduxjs / redux-toolkit

The official, opinionated, batteries-included toolset for efficient Redux development
https://redux-toolkit.js.org
MIT License
10.71k stars 1.17k forks source link

errors defininf middleware and preloadedState after upgrade to react 5 and react-redux 9 #3966

Closed kirill-linnik closed 10 months ago

kirill-linnik commented 10 months ago

After redux/react-redux upgrade, got errors on lines 25 and 36 while configuring store:

https://github.com/kirill-linnik/react-redux/blob/main/src/store/index.ts

const store = configureStore({
  reducer: rootReducer,
  middleware: (getDefaultMiddleware) => { // error 1
    let middleware = getDefaultMiddleware({
      serializableCheck: false, //as we are using Map, it seems to be not serializable, so we have to remove this check here
    });

    if (process.env.NODE_ENV !== "production") {
      return middleware.concat(logger);
    }

    return middleware;
  },
  preloadedState: loadFromLocalStorage(), //error 2
});

image

any ideas why?

markerikson commented 10 months ago

I'll first note that you should not be putting non-serializable values like Maps into the Redux state.

For this error specifically: does it still happen if you comment out the middleware.concat(logger) line?

kirill-linnik commented 10 months ago

I'll first note that you should not be putting non-serializable values like Maps into the Redux state.

For this error specifically: does it still happen if you comment out the middleware.concat(logger) line?

On your first point: I know, while Map is a super-handy data structure and, with this flag, it works just fine.

On your second comment: yes, error disappears. Does it mean redux-logger is not compatible with the new redux ? If yes, what are the options?

EskiMojo14 commented 10 months ago

this looks more like an issue with the Tuple and configureStore's inference - it doesn't like that your callback will return different Tuple versions, because it seems to have inferred the generic as only one of them

normally wouldn't recommend using push for middleware, but since logger doesn't actually affect the dispatch type at all, and since you're adding it conditionally, this should work:

const store = configureStore({
  reducer: rootReducer,
  middleware: (getDefaultMiddleware) => { // error 1
    let middleware = getDefaultMiddleware({
      serializableCheck: false, //as we are using Map, it seems to be not serializable, so we have to remove this check here
    });

    if (process.env.NODE_ENV !== "production") {
-      return middleware.concat(logger);
+      middleware.push(logger);
    }

    return middleware;
  },
  preloadedState: loadFromLocalStorage(), //error 2
});
kirill-linnik commented 10 months ago

this looks more like an issue with the Tuple and configureStore's inference - it doesn't like that your callback will return different Tuple versions, because it seems to have inferred the generic as only one of them

normally wouldn't recommend using push for middleware, but since logger doesn't actually affect the dispatch type at all, and since you're adding it conditionally, this should work:

const store = configureStore({
  reducer: rootReducer,
  middleware: (getDefaultMiddleware) => { // error 1
    let middleware = getDefaultMiddleware({
      serializableCheck: false, //as we are using Map, it seems to be not serializable, so we have to remove this check here
    });

    if (process.env.NODE_ENV !== "production") {
-      return middleware.concat(logger);
+      middleware.push(logger);
    }

    return middleware;
  },
  preloadedState: loadFromLocalStorage(), //error 2
});

it is easy to fork and try, but, nah, not really: image

EskiMojo14 commented 10 months ago

ah, that'll be it - redux-logger is pulling its Middleware type from v4. Until the types package peer deps are updated for v5 you'd likely need to use a npm override or yarn resolution to make sure it gets its types from v5.

kirill-linnik commented 10 months ago

ah, that'll be it - redux-logger is pulling its Middleware type from v4. Until the types package peer deps are updated for v5 you'd likely need to use a npm override or yarn resolution to make sure it gets its types from v5.

wdym?

EskiMojo14 commented 10 months ago

see the discussion here https://github.com/reduxjs/redux-toolkit/issues/3959

kirill-linnik commented 10 months ago

see the discussion here #3959

Hmm, I added:

  "overrides": {
    "redux": "5.0.0"
  }

to packange.json but it doesn't change a thing for me

EskiMojo14 commented 10 months ago

did you run npm install after?

kirill-linnik commented 10 months ago

did you run npm install after?

I use yarn, but, yes: deleted yarn.lock, then yarn, then yarn start

EskiMojo14 commented 10 months ago

I don't think yarn uses the overrides key, you should use resolutions

kirill-linnik commented 10 months ago

I don't think yarn uses the overrides key, you should use resolutions

thanks!

I also had to adjust loading from store a bit. from:

const loadFromLocalStorage = (): RootState | undefined => {
  try {
    const stateStr = localStorage.getItem(LOCAL_STORAGE_LOCATION);
    return stateStr ? JSON.parse(stateStr) : undefined;
  } catch (e) {
    console.error(e);
    return undefined;
  }
};

to:

const loadFromLocalStorage = (): Partial<typeof rootReducer> => {
  try {
    const stateStr = localStorage.getItem(LOCAL_STORAGE_LOCATION);
    return stateStr ? JSON.parse(stateStr) : {};
  } catch (e) {
    console.error(e);
    return {};
  }
};

BTW, do you think getting redux-logger to use v5 is a real thing? seems like their last package update was 6 years ago. should I use any alternative?

markerikson commented 10 months ago

We don't maintain redux-logger. Frankly your best bet is to either fork it, or rely on the Redux DevTools.

EskiMojo14 commented 9 months ago

The types package for redux-logger has been updated to depend on Redux v5 :)