CPLAT-7979: Add SafeRenderManager #390

greglittlefield-wf · 2019-10-21T21:55:31Z

Motivation

There were some cases where top-level react_dom.render/.unmountComponentAtNode calls were being done from within the event handlers and lifecycle methods of React components, which encountered timing-related issues in React 16.

Changes

Add SafeRenderManager:

A class that manages the top-level rendering of a ReactElement into a given node,
with support for safely rendering/updating via render and safely unmounting via tryUnmount.

Content is also unmounted when this object is disposed.

This is useful in cases where react_dom.render or react_dom.unmountComponentAtNode
may or may not be called from React events or lifecycle methods, which can have
undesirable/unintended side effects.

For instance, calling react_dom.unmountComponentAtNode can unmount a component
while an event is being propagated through a component, which normally would never happen.
This could result in null errors in the component as the event logic continues.

SafeRenderManager uses a helper component under the hood to manage the rendering of content
via Component state changes, ensuring that the content is mounted/unmounted as it
normally would be.

Add tests
Adjust two lints

Release Notes

Add SafeRenderManager, a class that manages the top-level rendering of a ReactElement into a given node, with support for safely rendering, updating, and unmounting content.

Review

See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.

Please review:

QA Checklist

Tests were updated and provide good coverage of the changeset and other affected code
Manual testing was performed if needed
- Steps from PR author:
  - Testing branches pass when this is pulled in
- Anything falling under manual testing criteria outlined in CONTRIBUTING.md

Merge Checklist

While we perform many automated checks before auto-merging, some manual checks are needed:

A Client Platform member has reviewed these changes
There are no unaddressed comments - this check can be automated if reviewers use the "Request Changes" feature
For release PRs - Version metadata in Rosie comment is correct

aviary2-wf · 2019-10-21T21:55:53Z

Security Insights

No security relevant content was detected by automated scans.

Action Items

Review PR for security impact; comment "security review required" if needed or unsure
Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

aaronlademann-wf

Looking good! Just a couple of minor things.

lib/src/util/safe_render_manager/safe_render_manager_helper.dart

lib/src/util/safe_render_manager/safe_render_manager.dart

aaronlademann-wf

Nice work @greglittlefield-wf !!! All the shared tests are particularly detailed, which should provide consumers with great confidence to integrate this into their libs.

Aside from a few suggestions / questions... The copyright stuff needs to be added to the top of all the new files:

// Copyright 2019 Workiva Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//     http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

lib/src/util/safe_render_manager/safe_render_manager.dart

aaronlademann-wf · 2019-10-29T12:28:00Z

lib/src/util/safe_render_manager/safe_render_manager.dart

+        // Don't do this in a finally since onMaybeUnmounted can get called async.
+        callOnMaybeUnmounted(true);
+        rethrow;
+      }
+    } else {
+      try {
+        _unmountContent();
+      } finally {
+        callOnMaybeUnmounted(true);
+      }


Does the comment on line 162 apply to line 170?

No, since there isn't a onMaybeUnmounted callback being used here.

lib/src/util/safe_render_manager/safe_render_manager_helper.dart

aaronlademann-wf · 2019-10-29T12:34:10Z

lib/src/util/safe_render_manager/safe_render_manager_helper.dart

+
+  void tryUnmountContent({void onMaybeUnmounted(bool isUnmounted)}) {
+    setState(newState()..content = null, () {
+      onMaybeUnmounted?.call(state.content == null);


Out of curiosity - is it possible for state.content to be non-null within a callback of a call to setState that sets state.content to null?

Yes; this can happen if this setState is batched with another setState that sets it to something else.