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

Always pull force flag from component #1984

Merged
merged 1 commit into from
Oct 7, 2019
Merged

Conversation

marvinhagemeister
Copy link
Member

@marvinhagemeister marvinhagemeister commented Oct 7, 2019

The way MobX works is that every observed component returns false from shouldComponentUpdate unless props have changed. They than trigger the child renders themselves via direct forceUpdate calls.

Our issue was that we only set the force flag sort-of globally during a render and would ignore any children that have been enqueued via forceUpdate in the same render cycle. This PR addresses that by ensuring that the force flag is always the one from the current component.

Tasks:

  • Add a test case

Fixes #1983
Size -24 B 💯

@coveralls
Copy link

coveralls commented Oct 7, 2019

Coverage Status

Coverage increased (+1.3%) to 99.767% when pulling c3b9cd6 on force-mobx into 02f8473 on master.

The way MobX works is that every observed component returns false
from shouldComponentUpdate unless props have changed. They than
trigger the child renders themselves via direct forceUpdate calls.

Our issue was that we only set the force flag sort-of globally
during a render and would ignore any children that have been
enqueued via forceUpdate in the same render cycle. With these
changes we ensure that the force flag is always the one from
the current component.
Copy link
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

Great work! I wanted to dig into this later too hehe beat me to it! Hero! 💯

@marvinhagemeister marvinhagemeister merged commit 4db53a3 into master Oct 7, 2019
@marvinhagemeister marvinhagemeister deleted the force-mobx branch October 7, 2019 19:47
@cristianbote
Copy link
Member

Wow! 😁

@andrewiggins
Copy link
Member

@marvinhagemeister Woo! Love it when we can save bytes and fix bugs :)

In src/components.js:132 you clear the force flag: component._force = false. Should this be somewhere else that guarentees the flag is always cleared even for children of whose _force flag is true.

In the UT that you added, do we also need to clear the components _force flag? What I'm thinking happens is that both Inner and Outer queue up a rerender, but renderComponent is only called with Outer, because Inner's _dirty flag is set to false when Outer's rerender is processed. Since Inner never goes into renderComponent (the condition on src/component.js:198 prevents it).

I took a stab at this but wanted to drop a message here so someone can look at it in case I forget.

@marvinhagemeister
Copy link
Member Author

@andrewiggins Good catch! That's something I didn't think of. Maybe we just need to reset the flag at the end of diff(), once a component has finished rendering instead of in renderComponent

cristianbote pushed a commit that referenced this pull request Oct 8, 2019
The way MobX works is that every observed component returns false
from shouldComponentUpdate unless props have changed. They than
trigger the child renders themselves via direct forceUpdate calls.

Our issue was that we only set the force flag sort-of globally
during a render and would ignore any children that have been
enqueued via forceUpdate in the same render cycle. With these
changes we ensure that the force flag is always the one from
the current component.
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.

10.0.0 break context update with mobx.toJs usage
5 participants