disptach updated #1872
disptach updated #1872
Conversation
|
Thanks @Siddharth1605 can you add tests for this PR? |
Here https://docs.aurelia.io/community-contribution/building-and-testing-aurelia there are 4 kinds of test to be done, as I'm a newbie to this community, can you say what test I need to do? |
Right thanks for pointing that out and sorry for the confusion. There should be existing tests in a file state.spec.ts you only need to follow it to create new tests. |
| @@ -81,6 +81,9 @@ export class Store<T extends object, TAction = unknown> implements IStore<T> { | |||
| } else { | |||
There was a problem hiding this comment.
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.
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.
| @@ -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.
I also noticed that line 94 is incorrect. this._dispatching--; If I understand the code correctly, this._dispatching--; should only be called after the whole afterDispatch returns. this._dispatching--; seems to work like a lock to ensure only one dispatch is happening and all the other requests are chained since the afterDispatch consumes the next item from the queue.
There was a problem hiding this comment.
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:
- calling
setStatetoo early, if there's more state changes inafterDispatch(this includes the wrongthis._dispatching--) - calling
setStateonly once, it'll miss all the other changes inafterDispatch.
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.
On a 2nd thought, maybe we should set state after every action so that it'll be easier & more correct to implement.
Pull Request
📖 Description
• Identified the code responsible for queuing and executing dispatch requests
• Reproduced the issue by building a page that dispatches multiple actions
• Implemented a fix to ensure that all dispatch requests set the new value
• Tested the fix by reproducing the issue and verifying that all dispatch requests are published to subscribers
This change ensures that the recursive call is properly handled for both synchronous and asynchronous cases, and it also accounts for the case when there are no more queued dispatches.
🎫 Issues
#1871