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.

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

ComponentBase: Prevent _buildState re-entry #70

Closed
ms-markda opened this issue May 8, 2018 · 3 comments
Closed

ComponentBase: Prevent _buildState re-entry #70

ms-markda opened this issue May 8, 2018 · 3 comments

Comments

@ms-markda
Copy link
Collaborator

If _buildState causes a store to trigger, that might call back into the component's _buildState before the first one has finished/returned. That can really mess up state updates since the setState will arrive in the wrong order*. Also, both _buildState will probably see the same this.state as the "previous state" since setState is not guaranteed to update this.state synchronously. Note: This is only a problem if _buildState depends on the previous state, or causes a change in the outside world (discouraged). Otherwise, both _buildState should return the same thing.

Is the solution to just use setState(updater) in #69? Or do we need to track this problem explicitly at runtime? Our current solution is to forbid store triggering from getters. That is manually enforced and can be error-prone. We could add runtime checks for that.

*
Call stack: FooStore.trigger -> MyComponent._buildStateWithAutoSubscriptions (first) -> MyComponent._buildState -> FooStore.getFoo -> FooStore.trigger -> MyComponent._buildStateWithAutoSubscriptions (second) -> MyComponent._buildState

The MyComponent._buildStateWithAutoSubscriptions (second) will finish and call setState, and then the stack will unwind to allow the MyComponent._buildStateWithAutoSubscriptions (first) to finish and call setState.

@berickson1
Copy link
Collaborator

Interesting - we prevent re-entry on store triggers, we should probably do that for components too.

Either that or prevent trigger from any _buildState path

    private _resolveThrottledCallbacks = () => {
        // Prevent a store from trigginer while it's already in a trigger state
        if (this._isTriggering) {
            this._triggerPending = true;
            return;
        }

@berickson1
Copy link
Collaborator

@ms-markda I think this is fixed in #80

@ms-markda
Copy link
Collaborator Author

@berickson1 Indeed, StoreBase will finish all callbacks before calling them again. I was thinking of an explicit solution (e.g. in ComponentBase) so the code has this problem mentioned, but this implicit solution should work. So long as we are confident this problem will not regress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants