Fix NPE for mouse/touch handlers in StrictMode #657
Conversation
Tie up with setting up and cleaning up listeners to lifecycle methods under the assumption that lifecycle methods like `componentDidMount`/`componentWillUnmount` might be called multiple times for the same instance.
pradeepnschrodinger
requested review from
abhisheksingh2410,
miskreant,
mycrobe,
novakps,
tchoiSchrodinger,
wcjordan,
AmanGupta2708 and
karry08
|
@Tanner-MS , do you recall if the issue in #615 was in production? |
| } | ||
|
|
||
| _setupHandlers() { | ||
| if (!this._wheelHandler) { |
There was a problem hiding this comment.
I added guard checks to ensure we won't create handlers more than once.
This lets us call _setupHandlers multiple times.
| this._divRef && | ||
| this._divRef.removeEventListener('wheel', this._wheelHandler.onWheel, { | ||
| if (this._divRef) { | ||
| this._divRef.addEventListener('wheel', this._wheelHandler.onWheel, { |
There was a problem hiding this comment.
We don't need a guard check here cause the event handler is a static function reference, so multiple addEventListener calls don't add duplicate entries.
| @@ -1037,6 +1060,8 @@ class FixedDataTable extends React.Component { | |||
| this._divRef = div; | |||
| if (this.props.stopReactWheelPropagation) { | |||
| this._wheelHandler.setRoot(div); | |||
| } else { | |||
| this._wheelHandler.setRoot(null); | |||
There was a problem hiding this comment.
Picked this up along the way.
Not exactly sure how we'll hit this code, but logically this just resets the root node if the prop is also reset.
Yes this was with production code. |
Ah, then it's unlikely that it was the same issue mentioned here. Thanks for the confirmation. I also see that the lifecycle methods of the components touched in #615 setup/cleanup the handlers properly, so it shouldn't be an issue, even for later versions of React. |
|
Released with v1.2.12 |
Description
When React's StrictMode is turned on, certain methods like lifecycle methods, constructors, setState methods, etc will be double invoked (in dev mode).
In fact, in future versions, it's possible that the same instance might be mounted/"destroyed" multiple times.
In StrictMode, the order of constructor/Lifecycle methods being called changes to:
However, our code in FDT is written under the assumption that life cycle methods are just called once.
So the current code only initializes the mouse/touch handlers in the constructor, and destroys them in componentWillUnmount.
This leads to the second call to componentDidMount working with invalid mouse/touch handlers, thus causing a NPE.
Fix
The fix was to move out initialization and cleanup to separate methods, namely
_setupHandlersand_cleanupHandlers.Setting up the handlers will be done in both the
constructorandcomponentDidMountmethods.Cleaning up will be done in just the
componentWillUnmountmethod.I've also added guard checks to ensure we won't reinitialize/cleanup more than needed.
Motivation
Avoid errors in future versions of React/StrictMode.
Fixes #656.
How Has This Been Tested?
Tested in local examples as well as the repo linked in #656 (https://github.com/savaparoski/not-working)
Types of changes
Checklist: