feat: deprecate reducer in favor of actionReducer and streamReducer APIs #558
Conversation
There was a problem hiding this comment.
Do we even use the "action reducer" flavor of the current reducer with multiple action creators anywhere?
Seems to be a very niche thing if we do. Maybe we should actually limit it to 1.
BTW, someone should give you access to the rxbeach repo. I'm not an admin there so I can't.
|
@holographic-principle, I found at least 3 places in ardoq-front where we use the multiple action flavor because of the TS errors. I think it would be interesting to study our use to see if some patterns and perhaps a better API structure emerge. I'm hoping we can achieve full hot module reloading and fast refresh as discussed in #555. It looks like I have access to |
src/reducer.ts
Outdated
| reducer: Reducer<State, VoidPayload> | ||
| ): RegisteredReducer<State, VoidPayload>; | ||
| <State, Payload = VoidPayload>( | ||
| actionCreator: UnknownActionCreator | UnknownActionCreator[], |
There was a problem hiding this comment.
The code is quite clear, so I was comfortable making this change.
There was a problem hiding this comment.
Note, I've removed this option in the new actionReducer because:
- it was hardly if ever used in ardoq-front
- it was very annoying to type correctly, mostly forcing us to use
any
|
I'm awaiting merging this PR until I have free time to follow through in ardoq-front. |
|
Note to self and everyone interested.
|
|
@sseppola: I personally have a problem with exposing action$ in the first place. |
|
@rafaa2, I just noticed how we start a routine: const routineSubscription = subscribeRoutine(action$, theRoutine, error$);We basically have to expose action$ due to this, unless we make subscribeRoutine connect to action$ automatically. |
|
@sseppola very good point! And that was exactly my recommendation at first. |
sseppola
force-pushed
the
feature/deprecate_overloaded_reducer_creator
branch
from
ab9a2cd
to
9531ca0
Compare
|
This is ready to go now, I just have a question regarding naming before I move on: Technically our
|
| @@ -6,7 +6,7 @@ import { incrementMocks } from './internal/testing/mock'; | |||
| const { reducers, actionCreators, handlers } = incrementMocks; | |||
| const { actions, words, numbers, errors } = incrementMocks.marbles; | |||
| const reducerArray = Object.values(reducers); | |||
| const alwaysReset = reducer( | |||
| const alwaysReset = reducer<number, any>( | |||
There was a problem hiding this comment.
This is the problem with reducers that accept multiple actionCreators
This PR deprecates the overloaded
reducerapi in favor of clearer separate functions: one for action reducers and one for stream reducers because they serve different purposes, and deserve their own api and description.With the separation of
StreamReducerCreatorandActionReducerCreatortypes I was not able to get thereducertype to act the same. So when upgrading there is a new TS error when passing an array of action creatorsUnknownActionCreator[], which is trivially fixed by using theactionReducerinstead. This could be considered a "breaking" change, but the functionality is the same.