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

idea: new reaction with delayed dependency check #1811

Closed
wants to merge 1 commit into from

Conversation

mayorovp
Copy link
Contributor

@mayorovp mayorovp commented Nov 14, 2018

see issue #1805

The problem of unnecessary evaluations is early shouldCompute call. While reaction can be delayed via scheduler or onInvalidate - shouldCompute is always called immediatelly. But what if we delay this call?

New reaction class can be used in React observers this way:

this[mobxReaction] = new ReactionEx(`${initialName}#${rootNodeID}.render()`, () => this.setState({}));

// ...

shouldComponentUpdate: function(nextProps, nextState) {
    return !shallowEqual(this.state, nextState) 
        || !shallowEqual(this.props, nextProps)
        || this[mobxReaction].shouldCompute()
}

@urugator
Copy link
Collaborator

Like it.

Could shouldComponentUpdate be delayed so much that it would somehow mess with mobx synchronous nature?

Can we somehow skip prop/state check, when the component is invalidated via Mobx (specific state field perhaps)?

@mayorovp
Copy link
Contributor Author

mayorovp commented Nov 19, 2018

Could shouldComponentUpdate be delayed so much that it would somehow mess with mobx synchronous nature?

Computeds already can delay the shouldCompute call forever.

Can we somehow skip prop/state check, when the component is invalidated via Mobx (specific state field perhaps)?

We can move this[mobxReaction].shouldCompute() call before this checks.

@mweststrate
Copy link
Member

@mayorovp tip: if you fix the single test failing api.js, it will make this PR look more solid :). (PR's that have failing builds tend to be ignored, as they are assumed WIP)

@mayorovp
Copy link
Contributor Author

@mweststrate but this PR is WIP

@ItamarShDev
Copy link
Member

@mayorovp is it still WIP?

@urugator urugator mentioned this pull request Feb 18, 2019
15 tasks
@mweststrate
Copy link
Member

Closing (can be further discussed in #1805).

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

Successfully merging this pull request may close these issues.

4 participants