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

Change .forceUpdate() to participate in the update queue #1939

Merged
merged 7 commits into from
Sep 23, 2019

Conversation

developit
Copy link
Member

This changes forceUpdate so that it's no longer inherently synchronous, instead pushing updates into the same queue currently used for state.

This means the following produces a single render pass:

class Demo extends Component {
  componentDidMount() {
    this.setState({ a:'b' });
    this.forceUpdate(() => console.log('updated'));
    this.setState({ a: 'c' });
  }
  shouldComponentUpdate() {
    return false;
  }
}

// A single render is triggered, bypassing shouldComponentUpdate, with `{a:'c'}` as state.

I left a commented-out possible tweak that would still allow standalone forceUpdate() to be synchronous when nothing else is in the queue, but I'm not sure that's desirable.

@robertknight
Copy link
Member

Out of interest, do you (or anyone else) know how this behavior compares to React in its various rendering modes (normal, batched, concurrent etc.)?

@marvinhagemeister
Copy link
Member

I haven't compared the various render modes of React specifically, but I can confirm that they schedule forceUpdate in some sort of queue and process all updates in the expected order. My guess is that this should be consistent across render modes and that those only change how the queue is processed. I'm leaning a bit out of the window here though

@developit
Copy link
Member Author

@robertknight @marvinhagemeister yes I think this is somewhat analogous to batched mode. Certainly in name, the change here enables batching of forceUpdate, meaning the only way to produce a non-batched update is now a top-level render() call for the tree. The nice part there is that any components re-rendered during that render() call will be unmarked as dirty, and their existing forceUpdate()/setState() enqueued re-render will be ignored when triggered. Any portions of the tree that don't get updated from the top-level render will still be updated when the batched changes are scheduled for rendering.

@coveralls
Copy link

coveralls commented Sep 19, 2019

Coverage Status

Coverage increased (+0.001%) to 99.883% when pulling 5c4168a on forceupdate-queue-participation into 9eba4e0 on master.

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.

This is looking great all failing tests should be resolved by rerender addition in the tests. analogue to #1892 (might be able to cherry pick)

@marvinhagemeister
Copy link
Member

@developit Awesome, also +1 on the additional tree-shaking benefits :)

Now we just need to pass the failing tests and then we're good to go 🎉

@marvinhagemeister
Copy link
Member

@JoviDeCroock Good catch! I went ahead and added all missing rerender() calls. This fixed every failing test except one 🎉

There componentWillReceiveProps was erroneously called on setState due to forgetting to set the force flag. Pushed a commit for that and the CI should be green now 👍

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

Super awesome! This makes it a lot easier for the devtools to measure commits and other things. Love it 💯 👍

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.

I think it'd be good to change the setter in useReducer to forceUpdate now instead of setState({}) might save some bytes too

@marvinhagemeister
Copy link
Member

@JoviDeCroock just checked: Using forceUpdate instead of setState({}) in our hooks code would make it larger by +2 B

@marvinhagemeister marvinhagemeister marked this pull request as ready for review September 23, 2019 16:01
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.

5 participants