Skip to content
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.

Sign up for GitHub

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

Jump to bottom

disptach updated #1872

Closed
wants to merge 1 commit into from
Closed

disptach updated #1872

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/state/src/store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,9 @@ export class Store<T extends object, TAction = unknown> implements IStore<T> {
} else {
Copy link

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

Copy link
Member

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.

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);
Copy link

Choose a reason for hiding this comment

The 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, 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.

Copy link
Member

Choose a reason for hiding this comment

The 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:

  • calling setState too early, if there's more state changes in afterDispatch (this includes the wrong this._dispatching--)
  • calling setState only once, it'll miss all the other changes in afterDispatch.

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?

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that makes sense

Expand Down