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

Fix double re-render in LiftWrapper and migrate it to PureComponent #82

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

KatSick
Copy link
Collaborator

@KatSick KatSick commented Apr 7, 2021

what was done:

Context

In the previous LiftWrapper, we used setState method to re-render our component when we subscribe to new props AND initially render our component.
As an example: during the very first render, we start rendering our renderCache as null and then synchronously re-render it via setState . Since by design Focal does need sync observable emit, I optimized first render to render initial observable value without invoking setState reconciliation (via refs).

todo before merge:

  • add MR description about the issue and the fix (and change title)
  • [not that easy :(]add unit tests for re-renders (currently only renderToStaticMarkup is in unit-tests)
  • [no results] investigate, how to test "waste render" in react via unit-tests (see screenshots)
  • perform benchmarks
  • [it works, but hard to compare values between runs, since they are too small and there is a lot of deviatiosn] investigate elm benchmark

Double re-render fix demo:

Screenshot 2021-07-26 at 11 21 10
Screenshot 2021-07-26 at 11 20 23

Screen.Recording.2021-07-26.at.11.19.14.mov

@KatSick KatSick changed the title Move to LiftWrapperII. Fix waste re-render WIP: Move to LiftWrapperII. Fix waste re-render Apr 8, 2021
@KatSick KatSick changed the title WIP: Move to LiftWrapperII. Fix waste re-render WIP: Migrate LiftWrapper to React hooks and optimize very first render Apr 8, 2021
@Wenqer Wenqer changed the title WIP: Migrate LiftWrapper to React hooks and optimize very first render Migrate LiftWrapper to React hooks and optimize very first render May 13, 2021
@Wenqer
Copy link
Collaborator

Wenqer commented May 13, 2021

@KatSick benchmark results look awesome! thanks!

@blacktaxi what do you think about those changes? Are we safe to merge it now, or you have any other objections?

packages/focal/src/react/react.ts Outdated Show resolved Hide resolved
packages/focal/src/react/react.ts Outdated Show resolved Hide resolved
@KatSick KatSick changed the title Migrate LiftWrapper to React hooks and optimize very first render Fix double re-render in LiftWrapper and migrate it to PureComponent Jul 26, 2021
new RenderMany(this, newProps, n) // eslint-disable-line
break
setRenderCache(cache: RenderCache): void {
if (this.renderCache === cache) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make sense to check equality with !structEq(this.renderCache, cache) as well? Just prevent rendering of the same state twice? This probably should be handled by React itself, but for some reason, we do that in the latest version. https://github.com/grammarly/focal/pull/82/files?diff=split&w=1#diff-71010d1ee0e73a76461ec411a47605c1207b1982317b93bd4fdc7c35fb7c6b1fL403

}
// eslint-disable-next-line camelcase
UNSAFE_componentWillMount() {
this._subscribe(this.props)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to move subscribe into constructor to avoid dependency on legacy lifecycle?

In the article Update on async rendering React team provides two examples about pre-fetch and subscription.

When supporting server rendering, it’s currently necessary to provide the data synchronously – componentWillMount was often used for this purpose but the constructor can be used as a replacement.

The Adding event listener suggest to use componentDidMount with initial value - it's not our case. But, they ok with using componentDidUpdate instead of componentWillReceiveProps

@KatSick what do you think?

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.

3 participants