Skip to content
This repository was archived by the owner on Jul 9, 2025. It is now read-only.

Conversation

@yeze322
Copy link
Contributor

@yeze322 yeze322 commented Nov 19, 2019

Description

How does the redraw promblem happen
My observation was based on two scenarios: 1. change focus state by clicking a node 2. change node data by adding/deleting a node.
According to the profiler's result, every time we do those two operations, Visual Editor will be redrawn twice:

  • the first redraw is triggered by Shell state update (expected);
  • the second redraw, which is redundant, is triggered by the useEffect hook which syncs Shell props to local state(unexpected).

So the redraw problem is basically caused by the misused useEffect hook and unnecessary local states in Visual Editor.

Why there are 'useEffect' hooks
At the very beginning, Visual Editor didn't sync states except 'obi json' with Shell, so all its states were local. As the evolution of Visual Editor, we are syncing more states between Visual and Shell, as a result, we hoisted many local states to shell layer. Thus useEffect hook was used an observer to sync Shell props to Visual Editor's local states which will always trigger another Visual Editor rendering.

Solution
As describe before, It is obviously an anti-pattern usage of the useEffect according to the official doc, indicates our state management is not good enough. According to our discussion, the long term plan is, we migrate to the useReducer hook to follow a redux-like state management pattern.

In the short run, for fixing the redraw issue, the solution is to let Visual Editor be rendered only by props passed from Shell and remove some local states such as focusedId.

Previously, the state chain is:

Shell props => (render) => local state => (render)

now, the state chain is:

Shell props => (render)

It's a quite straight forward fix after we hoisted visual editor states to Shell, but I think it would be much clearer by providing more context about it.

Task Item

Fixes #1496

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code refactor (non-breaking change which improve code quality, clean up, add tests, etc)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Doc update (document update)

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have functionally tested my change

Screenshots

Please include screenshots or gifs if your PR include UX changes.

@yeze322 yeze322 changed the title Remove misused 'useEffect' hook in Visual Editor to fix redraw problem Remove misused 'useEffect' hook in Visual Editor to fix redraw problem (#1496) Nov 19, 2019
const focusedId = Array.isArray(focusedActions) && focusedActions[0] ? focusedActions[0] : '';

// NOTE: avoid re-render. https://reactjs.org/docs/context.html#caveats
const [context, setContext] = useState({
Copy link
Contributor Author

@yeze322 yeze322 Nov 19, 2019

Choose a reason for hiding this comment

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

Notes here: the usage of useState here was still correct before we hoisting the focusedId to Shell but not necessary any more after our hoisting PRs. Every time we change focused Id will trigger the Shell Api onFocus for updating our URL.

@cwhitten cwhitten changed the title Remove misused 'useEffect' hook in Visual Editor to fix redraw problem (#1496) fix: misused 'useEffect' hook in Visual Editor (#1496) Nov 19, 2019
@cwhitten cwhitten merged commit b98656b into microsoft:master Nov 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants