Skip to content
This repository has been archived by the owner on Feb 6, 2023. It is now read-only.

Commit

Permalink
Remove 'componentWillUpdate' under GK
Browse files Browse the repository at this point in the history
Summary:
The last thing we do inside 'componentWillUpdate' in DraftEditor is update the
'_latestEditorState'.

This was introduced in #666 and that
PR has a great explanation in the comments.

**BUT** when manually testing this, the bug with dead key deletion and korean
IME deletion not working was still happening, even with the current state of
things. So that was interesting. It doesn't seem to fix the thing we originally
intended, unless I'm missing something.

In a later change (https://our.intern.facebook.com/intern/diff/D4778712/)
passes latestEditorState to handlers, so that they can use the latest editor
state instead of stale editor state. This fixed many bugs in our own uses of
Draft.js.

And since there are many untested IME cases, it could be that some situations
are still improved by storing and tracking the '_latestEditorState'.

We already set '_latestEditorState' inside 'update' and that is always called
before 'componentWillUpdate'. My first approach is to just rely on that and
remove the updating in 'componentWillUpdate'.

Updating '_latestEditorState' in 'componentWillUpdate' does potentially give
us important info though; if the 'onChange' handler mutated the editorState
then we will need to use that, and we get it in 'componentWillUpdate'.

If we want to maintain this, then I propose the following;
- Move '_latestEditorState' into state in 'DraftEditor' and create a public
  getter method to access it.
- Update it in the new static method 'getDerivedStateFromProps' and then use
  the polyfill wrapper to make Draft.js backwards compatible.
I'd rather skip that if it turns out we don't rely on '_latestEditorState'
being updated after 'onChange' is called. With so many uses and so few tests,
the only way to know is to try it. :)

Follow-up action items:
- Add a unit test for latestEditorState being used and passed to handlers.
- Open the GK to 70% employees and let that sit for a couple of days, to verify
  that this doesn't cause major problems. Continue roll-out and remove GK.
- Add inline comments explaining why we track and use '_latestEditorState'.

Depends on D6866079

Reviewed By: sophiebits

Differential Revision: D6873303

fbshipit-source-id: 5df00598ea20d826f96d70eddcfe7043cf18a18b
  • Loading branch information
flarnie authored and facebook-github-bot committed Feb 5, 2018
1 parent d144883 commit 7885959
Showing 1 changed file with 8 additions and 1 deletion.
9 changes: 8 additions & 1 deletion src/component/base/DraftEditor.react.js
Original file line number Diff line number Diff line change
Expand Up @@ -383,11 +383,18 @@ class DraftEditor extends React.Component<DraftEditorProps, State> {
// For people in the GK, we will skip setting this flag.
this._blockSelectEvents = true;
}
this._latestEditorState = nextProps.editorState;
if (!gkx('draft_js_remove_componentwillupdate')) {
// we are using the GK to phase out setting this here
this._latestEditorState = nextProps.editorState;
}
}

componentDidUpdate(): void {
this._blockSelectEvents = false;
if (gkx('draft_js_remove_componentwillupdate')) {
// moving this here, when it was previously set in componentWillUpdate
this._latestEditorState = this.props.editorState;
}
this._latestCommittedEditorState = this.props.editorState;
}

Expand Down

0 comments on commit 7885959

Please sign in to comment.