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

CPLAT-3094 Lifecycle Error Boundary #175

Merged
merged 30 commits into from
Apr 17, 2019

Conversation

joebingham-wk
Copy link
Collaborator

Description

In order to allow for the possibility of React 16 error boundaries, we needed to add getDerivedStateFromError and componentDidCatch lifecycle events. However, we also needed to ensure that not every component becomes and error boundary. Consequently, skipMethods was introduced to registerComponent.

Changes

  • Add the lifecycle event for componentDidCatch.
  • Add the lifecycle event for getDerivedStateFromError.
  • Modify tests to allow for static lifecycle events.
  • Add increased logic for skipMethods.
  • Add _filterSkipMethods to ensure that a consumer cannot skip important lifecycle events.
  • Add logic in _dart_helpers.js removing lifecycle events based upon the final skipMethods list.
  • Add test coverage.
  • Add an example in react_test.dart

Testing Suggestions

  • CI Passes
  • Manually check the new example to ensure that it behaves as expected.
  • Verify adequate unit test coverage.

@joebingham-wk joebingham-wk changed the base branch from master to 5.0.0-wip March 29, 2019 20:44
@kealjones-wk
Copy link
Collaborator

kealjones-wk commented Apr 1, 2019

As an update with where this currently stands... This implementation is working but the error that we get in getDerivedStateFromError is a JsObject or NativeJavaScriptObject... See issue on dart-lang/sdk here: dart-lang/sdk#36363

We are able to get at the dart exception in ways laid out in the ticket but we are hoping to get sign off from the dart team before moving forward with that...

because this is branched from #160 that has not been merged yet a cleaner diff can be found here: kealjones-wk/react-dart@react-16-new-context...cleandart:CPLAT-3094-lifecycle-error-boundary

js_src/_dart_helpers.js Outdated Show resolved Hide resolved
lib/react.dart Outdated Show resolved Hide resolved
lib/react.dart Outdated Show resolved Hide resolved
lib/react.dart Outdated Show resolved Hide resolved
lib/react.dart Outdated Show resolved Hide resolved
test/ReactSetStateTestComponent2.js Outdated Show resolved Hide resolved
test/ReactSetStateTestComponent2.js Show resolved Hide resolved
test/lifecycle_test.dart Outdated Show resolved Hide resolved
test/lifecycle_test.dart Outdated Show resolved Hide resolved
test/lifecycle_test/component2.dart Show resolved Hide resolved
@aaronlademann-wf aaronlademann-wf added this to the 5.0.0 milestone Apr 4, 2019
aaronlademann-wf and others added 5 commits April 4, 2019 08:41
# Conflicts:
#	lib/react.js.map
#	lib/react_with_addons.js.map
#	lib/react_with_react_dom_prod.js.map
#	package-lock.json
…s/react16/CPLAT-3094-lifecycle-error-boundary
…ndart/react-dart into CPLAT-3094-lifecycle-error-boundary

Catch up local branch with remote
lib/react_client.dart Outdated Show resolved Hide resolved
lib/react_client/react_interop.dart Outdated Show resolved Hide resolved
lib/react_client/react_interop.dart Outdated Show resolved Hide resolved
lib/react_client.dart Outdated Show resolved Hide resolved
lib/react_client.dart Show resolved Hide resolved
lib/react_client/react_interop.dart Outdated Show resolved Hide resolved
lib/react_client/react_interop.dart Outdated Show resolved Hide resolved
test/lifecycle_test/component2.dart Show resolved Hide resolved
js_src/_dart_helpers.js Outdated Show resolved Hide resolved
lib/react_client.dart Show resolved Hide resolved
lib/react_client.dart Outdated Show resolved Hide resolved
lib/react_client/react_interop.dart Outdated Show resolved Hide resolved
lib/react_client/react_interop.dart Outdated Show resolved Hide resolved
test/lifecycle_test.dart Outdated Show resolved Hide resolved
test/lifecycle_test/component2.dart Outdated Show resolved Hide resolved
lib/react.dart Outdated Show resolved Hide resolved
js_src/_dart_helpers.js Show resolved Hide resolved
lib/react_client.dart Show resolved Hide resolved
Copy link
Collaborator

@greglittlefield-wf greglittlefield-wf left a comment

Choose a reason for hiding this comment

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

A few small things, but otherwise looking really solid!!

js_src/_dart_helpers.js Outdated Show resolved Hide resolved
lib/react_client/react_interop.dart Outdated Show resolved Hide resolved
lib/react_client/react_interop.dart Outdated Show resolved Hide resolved
lib/react_client.dart Show resolved Hide resolved
test/ReactSetStateTestComponent2.js Outdated Show resolved Hide resolved
test/lifecycle_test.dart Show resolved Hide resolved
test/lifecycle_test.dart Outdated Show resolved Hide resolved
test/lifecycle_test.dart Outdated Show resolved Hide resolved
test/lifecycle_test.dart Outdated Show resolved Hide resolved
test/lifecycle_test.dart Outdated Show resolved Hide resolved
Copy link
Collaborator

@greglittlefield-wf greglittlefield-wf left a comment

Choose a reason for hiding this comment

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

+10

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.

4 participants