-
-
Notifications
You must be signed in to change notification settings - Fork 147
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
disptach updated #1872
disptach updated #1872
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,6 +81,9 @@ export class Store<T extends object, TAction = unknown> implements IStore<T> { | |
} else { | ||
return afterDispatch(newState); | ||
} | ||
}else { | ||
// Ensure that in case of no queued dispatches, the result is returned | ||
return $state instanceof Promise ? $state.then(() => {}) : undefined; | ||
} | ||
}; | ||
const newState = reduce(this._state, action); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also noticed that line 94 is incorrect. this._dispatching--; If I understand the code correctly, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you are right. The lines 89 - 97 currently looks like this return newState.then($state => {
this._setState($state);
this._dispatching--;
return afterDispatch(this._state);
}, ex => {
this._dispatching--;
throw ex;
}); It seems to have 2 problems:
I think it should be changed to something like this: return newState.then($state => {
return afterDispatch(this._state);
}, ex => {
this._dispatching--;
throw ex;
}).then(state => {
this._setState(state);
this._dispatching--;
}); If you agree with this @gtroxler @Siddharth1605 , can you help with the tests for these changes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On a 2nd thought, maybe we should set state after every action so that it'll be easier & more correct to implement. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes that makes sense |
||
|
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 would also set the state on line 80 (instead of just calling afterDispatch($), i would previously call set_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.
the job of
afterDispatch
is to run the state through the whole action queue to get the final state.Setting state should only happen after the whole action queue has been exhausted.