-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
RFC createAsyncThunk: reject with typed value #393
RFC createAsyncThunk: reject with typed value #393
Conversation
Deploy preview for redux-starter-kit-docs ready! Built with commit 27509b6 https://deploy-preview-393--redux-starter-kit-docs.netlify.com |
44a5501
to
dc4512b
Compare
Cool, glad you two have been busy while I've been at work :) |
dc4512b
to
29dc0c9
Compare
Apparently api-extractor doesn't like that I added a method on createAsyncThunk. To be honest, I'm not even sure if I like that method on there, so I'll leave it complaining for the moment. PS: Just had an idea. I'll probably move |
Okay, moved it on the Still needs to be fixed for TS3.5, but I'll do that once we're completely finished with the API. |
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 27509b6:
|
Yep, makes sense to me. (And this is why I pushed to make it an options object instead of positional args. I didn't know we'd add this specific thing, but as soon as we started looking at more than just |
Sorry about the delay in providing feedback here. General feedbackThese changes are great and very straightforward to work with. I don't see any problems with how the implementation is done, but I'm running into type issues. I'll do my best to outline the things I've found. What I'm trying to achieve with
|
...
This should be fixed, thanks for noticing!
You are setting let error: AxiosError = err // cast for access to AxiosError properties which defaults to You can use
I'd do this the other way around: instead of For the "mini serializable error format" missing for rejected.error: yes, currently that is
I'd probably do something along the lines of if (fetchLiveCallsError.fulfilled.match(result)) {
//success
} else {
if (result.payload) {
// rejected with value
} else {
// rejected by throw
}
} See the bogus type-test I added for more explanation |
207e6d2
to
4ef1360
Compare
Thanks, this all works as expected now. The component usage is also straightforward as you explained and general implementation is really simple. |
Sorry, what's the question / suggestion around serialized errors? |
Sorry, my bad. Currently, -export const miniSerializeError = (value: any): any => {
+export const miniSerializeError = (value: any): SerializedError => {
if (typeof value === 'object' && value !== null) {
// ...
return simpleError
}
- return value
+ return { message: String(value) }
} |
If you feel that would be more consistent, sure, we can go with that. |
So I'm not really sure, are the CI fails just from immer not working with 3.5/3.6 right now? |
Yeah, I expect TS 3.5 and 3.6 to fail right now due to the Immer alpha. Can you update the docs as well? |
src/createAsyncThunk.ts
Outdated
payload: undefined, | ||
error: miniSerializeError(error), | ||
payload, | ||
error: miniSerializeError(error) || { message: 'Rejected' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can ever even hit the fallback object here, because miniSerializeError
will always at least return {message: String(value)}
.
@@ -173,9 +201,17 @@ export function createAsyncThunk< | |||
getState, | |||
extra, | |||
requestId, | |||
signal: abortController.signal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add that notional AbortController
pseudo-polyfill in this PR as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gonna do that in an extra PR in the next few days, I think this has been blocked long enough :)
6bd918d
to
b1f66ea
Compare
I just rewrote the history of this, so we can merge this without squashing to keep @msutkowski's contributions under his name. (old head) |
And we have a mocked AbortController. It looks good from the tests, but I have no IE at hand, so I can only test this in theory. Anyways, I guess we'll get that feedback from some alpha testers ^^. |
The AbortController seems to cost us quite a bit though, as that's not really good minimizable. Stats before last commit:
Stats after:
Argh, my fault. Waitasec. |
Hmm. Yeah. I wonder if it's partly because of the class being transpiled. Any way to mimic that as a plain object / function that might end up being smaller? |
I think I just forgot to remove the comment around my NODE_ENV check... checking it right now. |
177155c
to
41faf00
Compare
41faf00
to
27509b6
Compare
Yeah, now i'ts looking better:
Oh, by the way: the error message from the nonSerializableMiddleware seems to take up ~0.5k - maybe we should shorten that in the prod build? |
Both the immutable and serializable middleware are stripped out in prod entirely, so they'll never show up in the actual prod bundle. |
Nope, this is from
|
Oh dear. That shouldn't be in there. Tell you what. Let's get this PR merged in now. I'm at work and theoretically supposed to be doing something productive atm. Can you take a look at why that error is even ending up in the prod build and deal with it in a separate PR? |
Can do :) |
* Port ngrx/entity and add createAsyncThunk (#352) * Initial port of `@ngrx/entity` implementation * Remove deprecated addAll method * Port `@ngrx/entity` tests * Simplify immutable entity operations by wrapping with Immer * Don't overwrite state.ids if sorting order hasn't changed * Simplify state adapter logic using Immer - Removed all references to DidMutate enum - Removed unneeded logic that only checked if state was mutated * Add `isFSA` helper to createAction * Swap state operator order to `(state, arg)` and support FSAs - Swapped arguments to state operators so that they can be reused as mostly standard Redux reducers - Added a check to handle arg as either an FSA action or a value - Swapped argument order in all test cases - Added one test to provide reading payload from FSAs works * Add a test to verify adapter usage with createSlice * Document unexpected Immer behavior with nested produce calls * Quiet lint warnings in tests I have no idea why the NgRx code is mutating the Array prototype in the first place, but let's leave that there for now. * Export Entity types as part of the public API * Add createAsyncThunk * Export createAsyncThunk as part of the public API * Ignore VS Code folder * Mark new types as alpha * 1.3.0-alpha.0 * Remove `removeMany(predicate)` overload * Rework dispatched thunk action contents - Move args inside `meta` - Include contents directly as `payload` * Update public API types * typings experiment * Update createAsyncThunk tests to match API changes * Simplify entity ID type definitions * Add a basic request ID counter to createAsyncThunk * Add nanoid * Include requestId in payload creator args, and use nanoid * Hopefully fix type definitions for empty thunk action params - Made `ActionParams = void`, which allows not declaring any args in the payload creation function without TS complaining - Found out I can switch the args order back so it's `(args, other)` * Add overloads to make EntityAdapter methods createSlice-compatible The overloads that had `TypeOrPayloadAction<T>` were resulting in a payload of `undefined` for the associated action creator when passed directly as a case reducer to `createSlice`. Adding overloads that explicitly reference `PayloadAction<T>` allows the inference to work correctly so that action payloads are detected. * Add a test that combines slices, async thunks, and entities * Remove TS 3.3 and 3.4 from the Travis setup * Update public API * 1.3.0-alpha.1 * Rework createAsyncThunk error handling behavior - Removed `finished` action - Serialized `Error` objects to a plain object - Ensured errors in `fulfilled` dispatches won't get caught wrongly - Changed to re-throw errors in case the user wants to handle them * Update public API * 1.3.0-alpha.2 * createAsyncThunk return fulfilled/rejected action instead of re-… (#361) * createAsyncThunk return fulfilled/rejected action instead of re-trowing errors * add unwrapResult helper * add .abort() to the createAsyncThunk thunkAction (#362) * add .abort() to the createAsyncThunk thunkAction * per review comments * put `abort` on the promise returned by `dispatch(asyncThunk())` * remove reference to DOMException * simplify rejected action creator * fix error==undefined case, reduce diff * update api report * Add initial `getAsyncThunk` API docs and usage guide * Rename thunk types and fields and export SerializedError * Update public API * 1.3.0-alpha.3 * Initial fix for createAsyncThunk thunk types * Rework `createAsyncThunk` types to enable specifying getState type * Fix thunk test types * Update public API * 1.3.0-alpha.4 * manually import types to prevent a bundling issue * strongly type slice name (#354) * strongly type slice name * move new generic to the end and default it to string * use ThunkApiConfig for optional type arguments (#364) * 1.3.0-alpha.5 * Modify createStateOperator to detect and handle Immer drafts * Update link styling to match main Redux site * Update blockquote styling to match main Redux site * Update side category menu styling to match main Redux site * Consolidate Update generic type and remove unused overload * Update `combinedTest` based on `createStateOperator` fixes * Add API docs for `createEntityAdapter` * guess what time it is again - it's public API time! * 1.3.0-alpha.6 * Remove accidental yarn.lock * Try fixing Netlify deploys: 1 * Update DS to fix sidebar bug * Try forcing node version * createAsyncThunk improvements (#367) * prevent dispatching of further actions if asyncThunk has been cancelled, even if the payloadCreator didn't react to the `abort` request * * add race between payloadCreator and abortedPromise * simplify createAsyncThunk * remove complicated logic where an AbortError thrown from the `payloadCreator` could influence the return value * api report * doc examples for cancellation * Remove extraneous period from abort message * Reorder cancellation content and improve wording * Fix code padding color busted from DS alpha.41 * 1.3.0-alpha.7 * Update Docusaurus and add lockfile to 43 version (#369) * Update Docusaurus and add lockfile to 43 version * Fix lockfile * Update netlify.toml to remove Yarn command * Try forcing node version Co-authored-by: Mark Erikson <mark@isquaredsoftware.com> * Try adding the compressed-size-action (#372) * Fix potential entity bugs identified by code review - Comparer should always return a number for sorting - Fixed missed state arg in add/remove test - Added test to confirm expected ID change behavior - Fixed bug in updateMany where multiple renames of one ID led to corrupted values in entities table afterwards * do that public API thing * Document caveats with update operations Co-authored-by: Lenz Weber <mail@lenzw.de> Co-authored-by: Thibault Gouala <thibault.gouala@gmail.com> Co-authored-by: Alexey Pyltsyn <lex61rus@gmail.com> * 1.3.0-alpha.8 * remove `any` where applicable (#377) * remove `any` where applicable * re-add `| undefined`, remove review comments * Fork redux-devtools-extension (#384) * Only check format for Markdown files in /docs * Add TS port of redux-devtools extension and use it * Remove redux-devtools-extension dependency * Remove stray console logs from tests * Feature/immutable invariant (#381) * strongly type slice name (#354) * strongly type slice name * move new generic to the end and default it to string * Remove accidental yarn.lock * Update DS to fix sidebar bug * Update Docusaurus and add lockfile to 43 version (#369) * Update Docusaurus and add lockfile to 43 version * Fix lockfile * Update netlify.toml to remove Yarn command * Try forcing node version Co-authored-by: Mark Erikson <mark@isquaredsoftware.com> * Try adding the compressed-size-action (#372) * Port redux-immutable-invariant and update docs * Update lock * Keep immutable middleware interface types during build * Readd lock file * Add mention of being a fork of redux-immutable-state-invariant * Markdown formatting Co-authored-by: Thibault Gouala <thibault.gouala@gmail.com> Co-authored-by: Mark Erikson <mark@isquaredsoftware.com> Co-authored-by: Alexey Pyltsyn <lex61rus@gmail.com> * Immutable middleware cleanup (#385) * Inline tiny-invariant and json-stringify-safe * Remove unused deps * Tweak immutable middleware docs typos * TS dep updates (#386) * Update createEntityAdapter type reference * Update TypeScript and Prettier to latest * Prettier reformatting * 1.3.0-alpha.9 * update TS docs, add new 1.3.0 APIs (#388) * Docs: add info on how to type Meta in `createSlice` * Docs: better example for signal.addEventListener * docs: TS usage for `createAsyncThunk` * docs: TS docs for `createEntityAdapter` * Edit new TS usage docs Co-authored-by: Mark Erikson <mark@isquaredsoftware.com> * Clarify createAsyncThunk type handling * Use Immer 6 alpha (#396) * Use fork of nanoid * Remove nanoid * Update to Immer 6 alpha * Enable Immer 6's ES5 support * Add TS 3.8 coverage * 1.3.0-alpha.10 * Formatting * RFC createAsyncThunk: reject with typed value (#393) * createAsyncThunk: add rejectWithValue function * Update docs on createAsyncThunk usage, add tests for rejectWithValue, fix rejected error return * implement AbortController stub for node, react native & IE Co-authored-by: Matt Sutkowski <msutkowski@gmail.com> * remove createSerializableStateInvariantMiddleware and createImmutableStateInvariantMiddleware functionality from production builds (#406) * bump immer to v6 (#407) * Immer 6 final (#409) * Update to Immer 6.0.1 * Fix AbortController test * Ignore serializability of `meta.args` by default (#410) * 1.3.0-beta.0 * display a warning if `immutableStateInvariantMiddleware` or `ser… (#427) * display a warning if `immutableStateInvariantMiddleware` or `serializableStateInvariantMiddleware` take too long * Update src/utils.ts Co-authored-by: Mark Erikson <mark@isquaredsoftware.com> * simplify signatures of `ActionCreatorWithOptionalPayload` and `A… (#428) * prevent any-casting of S generic on entityAdapter reducer-like f… (#436) * prevent any-casting of S generic on entityAdapter reducer-like functions * remove `map` from entityAdapter * remove references to `map` from the docs * update API report * remove export * Fix bug in docs for createAsyncThunk example (#417) fetchUserById payloadCreator should fetch when in pending (not idle) state * Feature/entity selectors (#440) * Add a `selectById` selector * Export Reselect types * Update API * 1.3.0-beta.1 * Update list of APIs and README Co-authored-by: Lenz Weber <mail@lenzw.de> Co-authored-by: Thibault Gouala <thibault.gouala@gmail.com> Co-authored-by: Alexey Pyltsyn <lex61rus@gmail.com> Co-authored-by: Lenz Weber <lenz.weber@mayflower.de> Co-authored-by: Matt Sutkowski <msutkowski@gmail.com> Co-authored-by: David Walsh <dlwalsh@gmail.com>
Starting from #390, @msutkowski and I have been discussing this further over discord.
This would be one possible way to reject from a
payloadCreator
, but pass typed data (which would be impossible usingthrow
, as that cannot be typed):Somewhere within
payloadCreator
, an object passed throughrejectWithValue
is returned. That object then ends up aspayload
of arejected
action that is finally dispatched & returned by the async thunk.See
https://github.com/phryneas/redux-toolkit/blob/44a550193646d309931405a107d66ea350f70f43/type-tests/files/createAsyncThunk.typetest.ts#L104-L123
This PR is essentially a starting point so @msutkowski can experiment with it in his example app.