Add `idGenerator` option to `createAsyncThunk` (#743) #976

Shrugsy · 2021-04-02T14:18:42Z

Closes #743

This PR adds an optional idGenerator option to the createAsyncThunk options to allow specifying how request IDs are generated rather than using nanoid.

netlify · 2021-04-02T14:21:22Z

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

Built with commit 62d1deb

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

codesandbox-ci · 2021-04-02T14:21:56Z

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 62d1deb:

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

* Make `idGenerator` description consistent with other sections of docs for `createAsyncThunk` * Re-arrange tests for `idGenerator`

markerikson · 2021-04-02T15:01:35Z

docs/api/createAsyncThunk.mdx

@@ -89,6 +89,7 @@ An object with the following optional fields:

 - `condition`: a callback that can be used to skip execution of the payload creator and all action dispatches, if desired. See [Canceling Before Execution](#canceling-before-execution) for a complete description.
 - `dispatchConditionRejection`: if `condition()` returns `false`, the default behavior is that no actions will be dispatched at all. If you still want a "rejected" action to be dispatched when the thunk was canceled, set this flag to `true`.
+- `idGenerator`: a function to use when generating the `requestId` for the request sequence. Defaults to use [nanoid](https://www.npmjs.com/package/nanoid).


✋ Let's point this to ./otherExports.mdx#nanoid instead, and maybe also tweak the wording of the nanoid paragraph to say it's the "default" instead of "automatically used"

Both points now addressed

I realised I made a sentence for the nanoid description incorrect when I adjusted it, but tweaked it to make sense again this time.

markerikson · 2021-04-02T15:02:40Z

src/createAsyncThunk.ts

@@ -392,7 +399,7 @@ If you want to use the AbortController to react to \`abort\` events, please cons
    arg: ThunkArg
  ): AsyncThunkAction<Returned, ThunkArg, ThunkApiConfig> {
    return (dispatch, getState, extra) => {
-      const requestId = nanoid()
+      const requestId = (options?.idGenerator || nanoid)()


✋ We can use nullish coalescing here instead of ||, right?

How exactly are you thinking? As in options?.idGenerator?.() || nanoid()?

(options?.idGenerator ?? nanoid)()

just swapping || for ??. same behavior, slightly more semantically correct.

Sorry just realised you said nullish coalescing (??). Definitely makes more sense, will do

Now addressed

msutkowski

Looks good, just have a few comments.

msutkowski · 2021-04-02T15:01:23Z

docs/api/createAsyncThunk.mdx

@@ -89,6 +89,7 @@ An object with the following optional fields:

 - `condition`: a callback that can be used to skip execution of the payload creator and all action dispatches, if desired. See [Canceling Before Execution](#canceling-before-execution) for a complete description.
 - `dispatchConditionRejection`: if `condition()` returns `false`, the default behavior is that no actions will be dispatched at all. If you still want a "rejected" action to be dispatched when the thunk was canceled, set this flag to `true`.
+- `idGenerator`: a function to use when generating the `requestId` for the request sequence. Defaults to use [nanoid](https://www.npmjs.com/package/nanoid).


Suggested change

- `idGenerator`: a function to use when generating the `requestId` for the request sequence. Defaults to use [nanoid](https://www.npmjs.com/package/nanoid).

- `idGenerator`: a function to use when generating the `requestId` for the request sequence. Defaults to use [nanoid](./otherExports.mdx#nanoid).

We don't use all of nanoid, so we should just link back specifically to what we inline and re-export

Yeah, but the link should go to the sibling Markdown file instead of the website directly :)

i'm fired 😂

Now addressed

src/createAsyncThunk.ts

Shrugsy · 2021-04-02T15:09:01Z

I'm struggling to understand why the preact step in the CI build is failing here: https://github.com/reduxjs/redux-toolkit/pull/976/checks?check_run_id=2254585946#step:3:265

  src/createAsyncThunk.test.ts:706:9 - error TS2345: Argument of type '{ idGenerator: jest.Mock<string, []>; }' is not assignable to parameter of type 'AsyncThunkOptions<void, {}>'.  
  Object literal may only specify known properties, and 'idGenerator' does not exist in type 'AsyncThunkOptions<void, {}>'.  

 706       { idGenerator }

As far as I'm aware I added idGenerator to the AsyncThunkOptions definition, including the api:

createAsyncThunk.ts

redux-toolkit.api.md

Does anyone have any insight as to what might be causing this error? Have I missed something somewhere?

markerikson · 2021-04-02T15:12:57Z

I think I saw the compressed size check also failing on some TS type changes with one of my other PRs this week. Not sure what's causing that.

* Link to local documentation for nanoid rather than npm package * Update local docs for `nanoid` to reflect changes to `createAsyncThunk` options * Use nullish coalescing operator when getting requestId generator

msutkowski · 2021-04-02T15:26:41Z

This line in the cAT docs will also need to be updated:

requestId: a unique string ID value that was automatically generated to identify this request sequence`

markerikson · 2021-04-02T15:28:29Z

Eh, I think that line's fine. There's still a requestId, and it's still a string, and it's still automatically generated - it's just a question of what the string contents are.

Shrugsy · 2021-04-02T15:40:40Z

Regarding the requestId description on the cAT docs: I think it is still accurate as it is. It could be extended to say something like "If an idGenerator function is provided, it uses that to generated the ID instead", but that would just essentially be repeating what is in the 'Options' section shortly after it.

Regarding the Compressed size check failing - Is there anything I can do about that? As far as I can tell, the failure it's hitting just shouldn't be happening. The same test that fails there passes in every other check.

Regarding comment threads, do you folks prefer:

for PR creators to advise that they addressed it, and leave it to the 'comment thread starter' to actually confirm it was addressed and press 'resolve conversation'?
or
for PR creators to decide for themselves that it was resolved, and press 'resolve conversation'?

markerikson · 2021-04-02T23:39:53Z

Y'know what, this looks good. Let's put it in.

Add idGenerator option to createAsyncThunk (reduxjs#743)

7d9662d

Tweak bad types used in tests (reduxjs#743)

22cf169

Shrugsy changed the title ~~Add idGenerator option to createAsyncThunk (#743)~~ [DRAFT] Add idGenerator option to createAsyncThunk (#743) Apr 2, 2021

Misc cleanup (reduxjs#743)

b50f095

* Make `idGenerator` description consistent with other sections of docs for `createAsyncThunk` * Re-arrange tests for `idGenerator`

markerikson requested changes Apr 2, 2021

View reviewed changes

msutkowski reviewed Apr 2, 2021

View reviewed changes

Address PR comments (reduxjs#743)

4227086

* Link to local documentation for nanoid rather than npm package * Update local docs for `nanoid` to reflect changes to `createAsyncThunk` options * Use nullish coalescing operator when getting requestId generator

Shrugsy changed the title ~~[DRAFT] Add idGenerator option to createAsyncThunk (#743)~~ Add idGenerator option to createAsyncThunk (#743) Apr 2, 2021

Fix wording for amended nanoid description (reduxjs#743)

62d1deb

markerikson merged commit 309c4bd into reduxjs:master Apr 2, 2021

Shrugsy deleted the 743-add-idGenerator-option-to-createAsyncThunk branch April 3, 2021 00:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `idGenerator` option to `createAsyncThunk` (#743) #976

Add `idGenerator` option to `createAsyncThunk` (#743) #976

Shrugsy commented Apr 2, 2021

netlify bot commented Apr 2, 2021 •

edited

codesandbox-ci bot commented Apr 2, 2021 •

edited

markerikson Apr 2, 2021

Shrugsy Apr 2, 2021

Shrugsy Apr 2, 2021

markerikson Apr 2, 2021

Shrugsy Apr 2, 2021

markerikson Apr 2, 2021

Shrugsy Apr 2, 2021

Shrugsy Apr 2, 2021

msutkowski left a comment

msutkowski Apr 2, 2021 •

edited

markerikson Apr 2, 2021

msutkowski Apr 2, 2021

Shrugsy Apr 2, 2021

Shrugsy commented Apr 2, 2021

markerikson commented Apr 2, 2021

msutkowski commented Apr 2, 2021 •

edited by markerikson

markerikson commented Apr 2, 2021

Shrugsy commented Apr 2, 2021

markerikson commented Apr 2, 2021

	- `idGenerator`: a function to use when generating the `requestId` for the request sequence. Defaults to use [nanoid](https://www.npmjs.com/package/nanoid).
	- `idGenerator`: a function to use when generating the `requestId` for the request sequence. Defaults to use [nanoid](./otherExports.mdx#nanoid).

Add idGenerator option to createAsyncThunk (#743) #976

Add idGenerator option to createAsyncThunk (#743) #976

Conversation

Shrugsy commented Apr 2, 2021

netlify bot commented Apr 2, 2021 • edited

codesandbox-ci bot commented Apr 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msutkowski left a comment

Choose a reason for hiding this comment

msutkowski Apr 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shrugsy commented Apr 2, 2021

markerikson commented Apr 2, 2021

msutkowski commented Apr 2, 2021 • edited by markerikson

markerikson commented Apr 2, 2021

Shrugsy commented Apr 2, 2021

markerikson commented Apr 2, 2021

Add `idGenerator` option to `createAsyncThunk` (#743) #976

Add `idGenerator` option to `createAsyncThunk` (#743) #976

netlify bot commented Apr 2, 2021 •

edited

codesandbox-ci bot commented Apr 2, 2021 •

edited

msutkowski Apr 2, 2021 •

edited

msutkowski commented Apr 2, 2021 •

edited by markerikson