Action stream consolidation #324
Conversation
markwhitfeld
force-pushed
the
action_stream_consolidation
branch
2 times, most recently
from
4d6f6b4
to
456c6d9
Compare
markwhitfeld
changed the title
|
@amcdnl @deebloo @leon Please review. I think that this is ready for merge. There are some particular points of importance to note:
PS. I was tempted to do some further refactoring around the creation of the StateContext but given that there is an outstanding issue (#311 ) regarding the fact that updateState seem to mutate state I rather left that code so as to avoid merge conflicts. |
|
@markwhitfeld how would this work for something like debounce? the actions stream still doesn't seem to trigger the actual state changes. Maybe I am missing something though. ill be back home tomorrow and would be happy to hop on the call |
|
@deebloo I still need to wire up the stream directly to the handler. At the moment the stream is triggering the invokeActions call that the dispatcher used to call. I also see pushing the stream further into the invoke actions code as part of the Action Pipes work. If you see this as something that should be part of this PR then I am happy to continue along that path. My concern would just be around merge hell if this branch sticks around for too long. |
Its very important this happens after completion.
This is fine in my opinion as long as it does error. Also, just to double check if i do |
packages/store/src/actions-stream.ts
Outdated
| @@ -7,6 +7,7 @@ import { Observable, Subject } from 'rxjs'; | |||
| export enum ActionStatus { | |||
| Dispatched = 'DISPATCHED', | |||
| Completed = 'COMPLETED', | |||
| Canceled = 'CANCELED', | |||
| Errored = 'Errored' | |||
There was a problem hiding this comment.
Need to make Errored actually ERRORED.
There was a problem hiding this comment.
Done. This is actually related to part of issue #313
| return compose([ | ||
| ...plugins, | ||
| (nextState, nextAction) => { | ||
| if (nextState !== prevState) { |
There was a problem hiding this comment.
Given the complexity of this function now, we should break it out into its own method.
packages/store/src/dispatcher.ts
Outdated
|
|
||
| actionResult$ | ||
| .pipe( | ||
| delay(0) // Force completion onto new task to prevent completion from firing before dispatch event on subsequent observers |
packages/store/src/dispatcher.ts
Outdated
| .pipe( | ||
| delay(0) // Force completion onto new task to prevent completion from firing before dispatch event on subsequent observers | ||
| ) | ||
| .subscribe(ctx => { |
There was a problem hiding this comment.
Make this is lambda aka remove {
There was a problem hiding this comment.
Done, although I prefer having them in a statement block because you can then put a breakpoint there if necessary.
packages/store/src/dispatcher.ts
Outdated
| .subscribe(ctx => { | ||
| this._actions.next(ctx); | ||
| }); | ||
| /* |
packages/store/src/dispatcher.ts
Outdated
| exhaustMap((ctx: ActionContext) => { | ||
| switch (ctx.status) { | ||
| case ActionStatus.Completed: | ||
| return of(this._stateStream.getValue()); |
There was a problem hiding this comment.
So the subscribe result should not return a value. The reason is was before was the value is needed for the plugins next().subscribe(state). Not sure how we can make the dispatch().subscribe() not return anything but make the next().subscribe(state=> work too. cc/ @deebloo
There was a problem hiding this comment.
@amcdnl yeah we would have to change plugins. Which I am certainly open to
There was a problem hiding this comment.
I think that I will have to leave it like this for now so that I don't break the plugins.
packages/store/src/state-factory.ts
Outdated
| ); | ||
| }) | ||
| ) | ||
| .subscribe(ctx => { |
There was a problem hiding this comment.
Done, although I prefer having them in a statement block because you can then put a breakpoint there if necessary.
| this._actions | ||
| .pipe( | ||
| filter((ctx: ActionContext) => ctx.status === ActionStatus.Dispatched), | ||
| mergeMap(({ action }) => { |
There was a problem hiding this comment.
Are we sure we want mergeMap here and not concatMap?
There was a problem hiding this comment.
for this case I don't think it really matters. as long as it isn't switchMap
There was a problem hiding this comment.
@amcdnl Yes definitely! A ConcatMap here would cause an issue where the action completions would wait for any pending previous actions to complete. You would rather want to notify of completion straight away.
packages/store/src/state-factory.ts
Outdated
| this._connected = true; | ||
| } | ||
|
|
||
| private get rootStateOperations(): InternalStateOperations<any> { |
There was a problem hiding this comment.
Getters/Setters should be before methods.
There was a problem hiding this comment.
Done. I moved it to the top.
|
@amcdnl this will only happen if you dispatch the exact same object. const action = new Action();
store.dispatch(action);
store.dispatch(action) |
|
@deebloo - Makes sense. I think I'm ok with that. We could fingerprint each one but that might be overkill. |
(PS. 5 failing unit tests at this point)
(PS: 17 tests failing now! ) ( Seems some plugins may have synchronous order dependencies in their code )
The completions will return from within a different task
The completion events in the action stream will still return asynchronously
(2 tests failing at this point. We need a canceled Action Status!)
markwhitfeld
force-pushed
the
action_stream_consolidation
branch
from
4d6f6b4
to
709f460
Compare
|
@amcdnl I think that the PR is ready now, please review again. |
Introduced an OrderedSubject extension of Subject that forces subscribers to receive values in the order that they were pushed This allowed us to remove the delay(0) hack
markwhitfeld
force-pushed
the
action_stream_consolidation
branch
from
709f460
to
6f16968
Compare
packages/store/src/actions-stream.ts
Outdated
| @@ -16,11 +16,30 @@ export interface ActionContext { | |||
| action: any; | |||
| } | |||
|
|
|||
| export class OrderedSubject<T> extends Subject<T> { | |||
There was a problem hiding this comment.
@deebloo I think you might like this.
|
idk about not squashing. this is one feature and if it ever needs to be reverted or picked it will be difficult. All of the commits will be listed in the description |
|
Ya, I agree w/ @deebloo - I think squash is the right thing given all the commits. |
As discussed by the team, we need to consolidate the dispatch and action handler invocation to use the action stream as the mechanism for execution.
This is required for the implementation of Action Pipes (#192 ).
Note: Due to the large changes in this PR, please could you merge into master without squashing the commits. I have taken care to document the decisions along the way in each commit and to keep them relatively small. This will help with potential merge conflicts with other branches too.