Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add section to usage guide on working with non-serializable data #623

Merged
merged 5 commits into from Jun 21, 2020

Conversation

cloeper
Copy link
Contributor

@cloeper cloeper commented Jun 18, 2020

Adds

  • Section to usage guide on working with non-serializable data
  • Updates error message thrown when encountering non-serializable values to include a link to the new section

…updates error message in serializableStateInvariantMiddleware to link to new section in usage guide
@netlify
Copy link

netlify bot commented Jun 18, 2020

Deploy preview for redux-starter-kit-docs ready!

Built with commit 7755ffb

https://deploy-preview-623--redux-starter-kit-docs.netlify.app

@cloeper
Copy link
Contributor Author

cloeper commented Jun 18, 2020

So, there's a number of failing tests on master. About 10 tests fail, all look like this:

TypeScript diagnostics (customize using `[jest-config].globals.ts-jest.diagnostics` option):
src/configureStore.ts:178:5 - error TS2345: Argument of type 'DeepPartial<S>' is not assignable to parameter of type 'PreloadedState<S>'.

My changes have nothing to do with that, so I'm unsure how to proceed.

@markerikson
Copy link
Collaborator

Hmm. Looking at build failures listed in Travis, they specifically seem to be about the snapshots for the serializability middleware. Not immediately seeing any TS-related issues.

@markerikson
Copy link
Collaborator

Also, why is that "fixes" commit full of formatting changes? Are we flip-flopping Prettier versions between people's installs or something?

@cloeper
Copy link
Contributor Author

cloeper commented Jun 19, 2020

Also, why is that "fixes" commit full of formatting changes? Are we flip-flopping Prettier versions between people's installs or something?

I wish I could tell you. I've run yarn format so I would assume that would resolve any issue.

@cloeper
Copy link
Contributor Author

cloeper commented Jun 19, 2020

Also, I updated the snapshots, but I have no idea why the tests aren't passing in travis. The only tests that don't pass for me locally are the ones I mentioned earlier.

This repo just isn't behaving as I would expect it to and I don't have the time to figure it out. Feel free to take the changes from my commits and adding them in a different PR.

@phryneas
Copy link
Member

phryneas commented Jun 19, 2020

There was a rogue space. I pushed a fix, let's see if it works now.

Edit: rather than that, it was missing - which makes my commit message nonsensical. Oh well, it's getting squashed away anyways :D

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 7755ffb:

Sandbox Source
proud-smoke-1xfhu Configuration
delicate-framework-x3ic6 Configuration
hardcore-elion-qm6ct Configuration

Copy link
Member

@msutkowski msutkowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reads nicely and is good for covering actions that a lot of 3rd party libraries dispatch 👍 .

My only other thought is that we should consider addressing 'What to do if a 3rd party redux library inserts non-serializable data into your store'. In that case, they'd want to leverage ignoredPaths. Currently, it's usage is pretty opaque, but can be figured out.

Here is a quick draft of that idea:

Handling non-serializable data in an action

... original stuff

Handling non-serializable data in your store

In the instance that a 3rd party library is inserting non-serializable data into your store and you're unable to have this resolved with the library author, you can work around this by using the ignoredPaths property when configuring the store.

configureStore({
  //...
  middleware: getDefaultMiddleware({
    serializableCheck: {
      ignoredPaths: ['reducerKey'] // would target `state.reducerKey` and all of it's children
    }
  })
})

@markerikson
Copy link
Collaborator

markerikson commented Jun 19, 2020

I think we at least ought to have a specific example of setting up with Redux-Persist, because A) it's a legit example of customizing the behavior, and B) it's come up fairly frequently in the issues.

(Also maybe React-Redux-Firebase, if there's anything relevant there?)

@msutkowski
Copy link
Member

msutkowski commented Jun 19, 2020

@markerikson There for sure is with react-redux-firebase as they add Timestamp instances into the store and other instance types in almost every action. I've made a few contributions to that and redux-firestore and plan on opening a PR for their 4.0 release to address the serializable issue as all of that can be handled rather simply by calling instance.toString() or instance.toJSON() depending on the firebase instance type.

Either way, an example of the configuration needed for those libs looks like this:

import {
    configureStore,
    getDefaultMiddleware,
  } from "@reduxjs/toolkit";
  import {
    getFirebase,
    actionTypes as rrfActionTypes,
  } from "react-redux-firebase";
  import { constants as rfConstants } from "redux-firestore";
  import rootReducer from "./rootReducer";
  import { useDispatch } from "react-redux";

  const extraArgument = {
    getFirebase,
  };
  export type DefaultThunkExtra = typeof extraArgument;
  
  const middleware = [
    ...getDefaultMiddleware({
        serializableCheck: {
          ignoredActions: [
              // just ignore every redux-firebase and react-redux-firebase action type
            ...Object.keys(rfConstants.actionTypes).map(
              (type) => `${rfConstants.actionsPrefix}/${type}`
            ),
            ...Object.keys(rrfActionTypes).map((type) => `@@reactReduxFirebase/${type}`),
          ],
          ignoredPaths: ["firebase", "firestore"],
        },
        thunk: {
          extraArgument,
        },
      }),
  ] as const;
  
  const store = configureStore({
    reducer: rootReducer,
    middleware,
  });

  export type AppDispatch = typeof store.dispatch;
  export const useAppDispatch = () => useDispatch<AppDispatch>();
  
  export default store;

Note: there is more typing that needs to be done specifically for the reducers they ship to be TS compatible with RTK, but is probably a separate issue. I haven't seen many people mention that, but a PR I had opened for the problem is hopefully getting merged in their 3.6 release

Looking at that, we should probably add another property of ignoreActionPrefix for these scenarios (or some other partial matcher). I believe redux-websocket still has this same type of issue, although a more limited # of action types. I'm sure there's even more that I'm not aware of 🤷

@markerikson
Copy link
Collaborator

I'll go ahead and merge this, and do some more edits on the docs content from there. Thanks!

@markerikson markerikson merged commit fb22912 into reduxjs:master Jun 21, 2020
@markerikson
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants