-
-
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
alternative callback-builder-style notation for actionsMap #262
Conversation
Deploy preview for redux-starter-kit-docs ready! Built with commit a66f478 https://deploy-preview-262--redux-starter-kit-docs.netlify.com |
9b18b99
to
6de0dd0
Compare
|
||
builder.addCase( | ||
increment, | ||
(state, action: ReturnType<typeof increment>) => state |
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.
Somehow I'd actually kind of figure this action type could get inferred automatically, but maybe that's not the case.
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.
It does (see line 86), this is just there to test if it can actually also be specified without error.
Looks pretty neat. I assume we wouldn't want to allow this for |
Exactly. I'm seeing a use case for Maybe also for some kind of async middleware in the future. |
Yeah, go ahead and rig it up for
How so? |
Intersting sidenote: there's no codesandbox being generated for this. Maybe because it's a Draft PR? 🤔 |
The most simple async middleware I could think of would be something like an actionEffectsMap, either as an argument on configureStore({
// ...
asyncEffects: {
[todosRequested.type]: async (action: ReturnType<typeof todosRequested>, { getState, dispatch }) => {
// async stuff here
}
}
}); If we did something like that, that could benefit from a similar alternative notation. |
Hey, good point. And yes, a simple action -> callback map is basically what I'm picturing. Already opened up a thread to discuss it over in #237 . |
btw, after this, it'd be nice if we could add an actual "Usage with TypeScript" docs page that rephrases the info in the "Advanced Tutorial" and talks about using this builder. |
Yeah. Also, the docblocks will need some updating. Do you know how to correctly write docblocks for overloaded methods? |
Not specifically, no, and tbh the types I've listed in the API reference pages are sorta hand-wavy anyway (like references to a I'd say write the page for the callback builder first, then update the other couple pages to point to that and say that the args can either be a hand-written object or the result of the builder. |
…uilders.typetest.ts
Code-wise this should be it. I'm gonna take a look at the docs tomorrow - I should get some sleep now :) (Also, this should be a normal PR now, not a Draft PR - let's see what CodeSandbox does with it) |
@phryneas : maybe we lost the CodeSandbox integration due to the repo name change? |
I guess so. Could you remove & re-add the github integration for a test? I've now added the missing docblock comments. As for the documentation page, writing just doesn't come to me today. Otherwise, I'll try again in a few days. That writing mood is unpredictable. |
I didn't even set up the actual integration - I filled out the form and got an email from someone, I think. Lemme see if I can find that email and reply asking for an update. I'm trying to convince myself to work on a blogpost atm, so I'm gonna hold off on trying to write docs for this. Although having said that, the stuff I added to the Redux core "Usage with TS" looks fairly similar to where you're headed with that sandbox: https://redux.js.org/recipes/usage-with-typescript#usage-with-redux-toolkit And I like the outline that I see there. |
I just created a new issue for the TS usage page and copied these comments over there. |
I think you have to go to https://github.com/apps/codesandbox and re-activate it for the renamed sandbox. Aside from that - is this ready in your eyes/can I merge this or do you want to wait for the documentation to catch up? |
You'll need to enable the new repo in the CodeSandbox CI app - we only see reduxjs/redux-starting-kit enabled atm. |
Yeah, we really oughta merge this in :) Can you add info on this to the API docs as well? We can do that as a separate PR. |
@garethx by now, codesandbox builds are running again, but I just noticed that codeSandbox cannot find types on the CI builds. See this sandbox: https://codesandbox.io/s/friendly-hoover-g0sni |
This is what I had suggested on twitter. Right now, it's just implemented on createReducer, but if you agree that we go forward with this, I'll add it to extraReducers, too.