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 baseQueryMeta to thunks, meta to lifecycle results #1084

Merged
merged 4 commits into from
Jun 3, 2021

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented May 23, 2021

This contains #1083 and is a definite WIP, just want to see if it works for #1080

@TamasSzigeti could you give the CodeSandbox build of this a spin? Types are still missing, but meta is on there, so you might have to do an any-cast to get it out there atm. if working with TS

Install instructions at https://ci.codesandbox.io/status/reduxjs/redux-toolkit/pr/1084/

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 23, 2021

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 dcd4c8d:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration
rsk-github-issues-example Configuration

@github-actions
Copy link

Size Change: +1.97 kB (0%)

Total Size: 532 kB

Filename Size Change
dist/query/react/rtk-query-react.cjs.development.js 17.3 kB +90 B (+1%)
dist/query/react/rtk-query-react.cjs.production.min.js 11.1 kB +23 B (0%)
dist/query/react/rtk-query-react.esm.js 17 kB +95 B (+1%)
dist/query/react/rtk-query-react.modern.development.js 14 kB +50 B (0%)
dist/query/react/rtk-query-react.modern.js 14 kB +53 B (0%)
dist/query/react/rtk-query-react.modern.production.min.js 9.28 kB +33 B (0%)
dist/query/react/rtk-query-react.umd.js 187 kB +197 B (0%)
dist/query/react/rtk-query-react.umd.min.js 62.9 kB +79 B (0%)
dist/query/rtk-query.cjs.development.js 14.6 kB +79 B (+1%)
dist/query/rtk-query.cjs.production.min.js 9.37 kB +25 B (0%)
dist/query/rtk-query.esm.js 14.3 kB +84 B (+1%)
dist/query/rtk-query.modern.development.js 11.7 kB +55 B (0%)
dist/query/rtk-query.modern.js 11.7 kB +54 B (0%)
dist/query/rtk-query.modern.production.min.js 7.72 kB +26 B (0%)
dist/query/rtk-query.umd.js 27.2 kB +189 B (+1%)
dist/query/rtk-query.umd.min.js 16.1 kB +101 B (+1%)
dist/redux-toolkit.cjs.development.js 12.1 kB +106 B (+1%)
dist/redux-toolkit.cjs.production.min.js 5.87 kB +64 B (+1%)
dist/redux-toolkit.esm.js 11.7 kB +116 B (+1%)
dist/redux-toolkit.modern.development.js 9.6 kB +102 B (+1%)
dist/redux-toolkit.modern.js 9.64 kB +104 B (+1%)
dist/redux-toolkit.modern.production.min.js 4.58 kB +69 B (+2%)
dist/redux-toolkit.umd.js 22 kB +111 B (+1%)
dist/redux-toolkit.umd.min.js 10.8 kB +67 B (+1%)
ℹ️ View Unchanged
Filename Size Change
dist/index.js 146 B 0 B
dist/query/index.js 144 B 0 B
dist/query/react/index.js 149 B 0 B

compressed-size-action

@netlify
Copy link

netlify bot commented May 23, 2021

✔️ Deploy Preview for redux-starter-kit-docs ready!

🔨 Explore the source changes: dcd4c8d

🔍 Inspect the deploy log: https://app.netlify.com/sites/redux-starter-kit-docs/deploys/60b6a0dcb0f7700008d6f524

😎 Browse the preview: https://deploy-preview-1084--redux-starter-kit-docs.netlify.app

@TamasSzigeti
Copy link
Contributor

Yes I'm using TypeScript and seems the parts relying on createAsyncThunk are having type errors now. Specifically in extraReducers where I'm using builder.addCase(someThunk.fulfilled, (state, { payload }) => { … }), it results in payload being typed as unknown.
I can try to spend more time on it tomorrow to understand it better.

@phryneas
Copy link
Member Author

@TamasSzigeti very valuable feedback, thank you. I will look into that & add a test for it.

@phryneas
Copy link
Member Author

@TamasSzigeti I have added a test for that over at 64a8c64#diff-a11e9f94b91b0761740b0b3daf0f10c2b45a1881e554e229914dc23d146826e1R104 but I cannot really reproduce that behaviour. Can you please create a reproduction?

@TamasSzigeti
Copy link
Contributor

@TamasSzigeti Can you please create a reproduction?

@phryneas I trimmed down my case, and it seems that the unknown payload comes about when I pass a second argument to the payloadCreator, whether I want to use dispatch or getState. When I leave that argument out, as the case in the tests, the return type is propagating just fine.

https://gist.github.com/TamasSzigeti/2ddade02d2e647d25d8810da04dba494

@phryneas
Copy link
Member Author

Thank you @TamasSzigeti !
That one was actually super tricky to find & solve, but I think I got it. Can you try it again?

Thanks for the report 👍 😃

@phryneas phryneas changed the base branch from feature/v1.6-integration to feature/cAT-meta May 27, 2021 21:49
@github-actions
Copy link

github-actions bot commented May 27, 2021

size-limit report 📦

Path Size
1. entry point: @reduxjs/toolkit (cjs.production.min.js) 12.16 KB (0%)
1. entry point: @reduxjs/toolkit (esm.js) 10.19 KB (0%)
1. entry point: @reduxjs/toolkit/query (cjs.production.min.js) 20.09 KB (+0.16% 🔺)
1. entry point: @reduxjs/toolkit/query (esm.js) 17.11 KB (+0.19% 🔺)
1. entry point: @reduxjs/toolkit/query/react (cjs.production.min.js) 21.87 KB (+0.18% 🔺)
1. entry point: @reduxjs/toolkit/query/react (esm.js) 19.46 KB (+0.2% 🔺)
2. entry point: @reduxjs/toolkit (without dependencies) (cjs.production.min.js) 5.51 KB (0%)
2. entry point: @reduxjs/toolkit (without dependencies) (esm.js) 5.48 KB (0%)
2. entry point: @reduxjs/toolkit/query (without dependencies) (cjs.production.min.js) 8.98 KB (+0.38% 🔺)
2. entry point: @reduxjs/toolkit/query (without dependencies) (esm.js) 9.38 KB (+0.4% 🔺)
2. entry point: @reduxjs/toolkit/query/react (without dependencies) (cjs.production.min.js) 2.25 KB (0%)
2. entry point: @reduxjs/toolkit/query/react (without dependencies) (esm.js) 2.1 KB (0%)
3. createSlice (esm.js) 5.15 KB (0%)
3. createEntityAdapter (esm.js) 5.19 KB (0%)
3. configureStore (esm.js) 5.2 KB (0%)
3. createApi (esm.js) 15.73 KB (+0.21% 🔺)
3. createApi (react) (esm.js) 18.01 KB (+0.16% 🔺)
3. fetchBaseQuery (esm.js) 10.6 KB (-0.06% 🔽)
3. setupListeners (esm.js) 9.75 KB (-0.07% 🔽)
3. ApiProvider (esm.js) 16.98 KB (+0.15% 🔺)

@TamasSzigeti
Copy link
Contributor

That one was actually super tricky to find & solve, but I think I got it. Can you try it again?

Nice, thank you! My case works perfectly having re-implemented it with onCacheEntryAdded. Meta is there, only the typing is missing.
Plus the serializability middleware is complaining about the meta field in the api/executeQuery/fulfilled action payload:

A non-serializable value was detected in an action, in the path: `meta.baseQueryMeta.request`

@phryneas
Copy link
Member Author

Yup, the types will be the next step :) Thanks for validating!

@phryneas phryneas changed the base branch from feature/cAT-meta to feature/v1.6-integration May 31, 2021 20:47
`meta.arg.startedTimeStamp` -> `meta.startedTimeStamp`
`payload.fulfilledTimeStamp` -> `meta.fulfilledTimeStamp`
`payload.result` -> `payload`
@phryneas
Copy link
Member Author

phryneas commented Jun 1, 2021

Aaand we have types.

@TamasSzigeti
Copy link
Contributor

Awesomeness, and it works perfectly, same types as with onSuccess before. One thing that might worth adding to the docs is a note about when can the meta come through as undefined. I guess it has to do with an overridden queryFn?

Thanking muchly, this is becoming nice and clean, very pleasant to work with.

@markerikson markerikson marked this pull request as ready for review June 3, 2021 00:20
@markerikson markerikson merged commit ada1f4b into feature/v1.6-integration Jun 3, 2021
@markerikson markerikson deleted the feature/thunk-shape branch June 3, 2021 00:28
@phryneas
Copy link
Member Author

phryneas commented Jun 3, 2021

@TamasSzigeti Those are always optional because I did not want to add multiple generic arguments here for something that is more of an "edge case" use.
Depending on your baseQuery, it might only return meta for successes, only for errors, or even none or different ones. I've put these into one "combined" generic argument to reduce complexity of the types (which already are insane anyways).

In the case of fetchBaseQuery, they should always be there with the maximum available information unless some unexpected error happened there and it actually crashed.

@TamasSzigeti
Copy link
Contributor

@phryneas Thanks, my interpretation was correct then. My point is that this could be useful to be added in the docs.

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.

3 participants