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

createAsyncThunk cannot pass non-serializable arg #453

Closed
terrierscript opened this issue Mar 28, 2020 · 6 comments · Fixed by #457
Closed

createAsyncThunk cannot pass non-serializable arg #453

terrierscript opened this issue Mar 28, 2020 · 6 comments · Fixed by #457

Comments

@terrierscript
Copy link

terrierscript commented Mar 28, 2020

I try to use createAsyncThunk with non-serializable object like:

export const roundAsync = createAsyncThunk(
  "someAsync",
  ({ api }: { api: ApiClient }) => {
    return api.apiAction()
  }
)

and I got error

index.js:1 A non-serializable value was detected in an action, in the path: `meta.arg.api`. Value: ApiClient {} 
Take a look at the logic that dispatched this action:

I found this issue ignored meta.args by default
#410

but maybe createAsyncThunk fire meta.arg

Additionaly, I try customize serializableCheck.ignoredPaths option, I cannot resolve this too.
This maybe not passed ignorePaths params to findNonSerializableValue

const foundActionNonSerializableValue = findNonSerializableValue(
action,
[],
isSerializable,
getEntries
)

@markerikson
Copy link
Collaborator

markerikson commented Mar 28, 2020

Eep. Yeah, we originally called that meta.args, but it looks like at some point in the development process that got renamed to meta.arg instead.

@phryneas , which way do we want to call it? We either need to either keep meta.arg and change the serialization ignore field to match, or rename to meta.args.

Hmm. You've also got a good point on the paths thing. Now I'm confused why our tests didn't catch this.

@phryneas
Copy link
Member

@markerikson I think the renaming to meta.arg was done as the asyncThunk takes only one arg, not multiple ones. So by that logic I'd opt for keeping meta.arg and changing the field match.

@markerikson
Copy link
Collaborator

Okay. You got time to make that edit and fix whatever tests aren't catching this?

We should probably use createAsyncThunk in the test - without looking at it, I bet it's a hand-written action, so this change didn't get caught.

@phryneas
Copy link
Member

I can take a look tomorrow.

@phryneas
Copy link
Member

phryneas commented Mar 29, 2020

Okay, looking at the code I have questions.

Right now, it looks like we are using ignoredPaths only for state paths and then use the default value to put in meta.args (or in the future, arg) for action paths.
Which means, ignored action paths are not overridable. Also, there's just no test for action paths, since those weren't overridable and I believe we just patched that in somehow a while ago.

While I'm at it: Want to make it possible to override action paths by doing an ignoredPath like ["action.meta.arg"]? (Alternatively: add new parameter ignoredActionPaths)

@phryneas
Copy link
Member

Let's move that discussion into the PR (if there's any reason for discussion :) )

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 a pull request may close this issue.

3 participants