Skip to content

Conversation

@aduth
Copy link
Member

@aduth aduth commented Aug 7, 2017

This pull request seeks to add error handling to block edit rendering. Error boundaries are a new feature added in React 16 which add a simple componentDidCatch lifecycle hook to capture errors which occur during a component's rendering. With these changes, an error boundary wraps rendering of each block and captures errors, presenting a warning message if one occurs, rather than allowing it to go unhandled and crash the entire application.

Before After
Before After

Implementation notes:

The original implementation attempt applied componentDidCatch to VisualEditorBlock itself but this had some odd effects on componentDidMount and refs, so it was later revised to create a separate wrapping error boundary component.

Testing instructions:

  1. Navigate to Gutenberg > Demo, or Gutenberg > New Post
  2. In your browser developer tools console, register the Mr. Crashy block:
    • wp.blocks.registerBlockType( 'myplugin/mr-crashy', { title: 'Mr. Crashy', category: 'common', icon: 'warning', edit() { throw new Error(); }, save() { return ''; } } );
  3. From the inserter menu, select Mr. Crashy
  4. Note that the block is inserted and displays a warning about failure

Open questions:

  • What do we do about saved content? If an error occurs after making some revisions, the changed block attributes are still reflected in the saved output. To me, this seems reasonable, but is that expectation shared?
  • How can we surface the actual error message that was captured? Might be better to be addressed in a subsequent pull request.

aduth added 3 commits August 7, 2017 10:34
It doesn't necessarily follow that this should be necessary, since refs should be assigned during componentDidMount. Specifically in the case that the error boundary catches an error though, the ref is not assigned. This may be related to how React, by its error message, "recreate[s] this component tree from scratch". Do we need a separate component for error boundary rendered as a wrapper of the block edit function? Might interfere with styling.
See e94b219

Rendering the crash boundary as a wrapper to the block edit allows node ref to be assigned by VisualEditorBlock normally.
@aduth aduth added the [Feature] Blocks Overall functionality of blocks label Aug 7, 2017
@codecov
Copy link

codecov bot commented Aug 7, 2017

Codecov Report

Merging #2267 into master will decrease coverage by 0.1%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2267      +/-   ##
=========================================
- Coverage    23.9%   23.8%   -0.11%     
=========================================
  Files         144     147       +3     
  Lines        4530    4550      +20     
  Branches      766     771       +5     
=========================================
  Hits         1083    1083              
- Misses       2912    2927      +15     
- Partials      535     540       +5
Impacted Files Coverage Δ
editor/modes/visual-editor/block-crash-warning.js 0% <0%> (ø)
editor/modes/visual-editor/create-block-warning.js 0% <0%> (ø)
editor/modes/visual-editor/block.js 0% <0%> (ø) ⬆️
editor/modes/visual-editor/block-crash-boundary.js 0% <0%> (ø)
...ditor/modes/visual-editor/invalid-block-warning.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3084b9...29eeb43. Read the comment docs.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

This is a really awesome React Feature.
it works great.

I also find it reasonable to continue running the save function like any other block. An alternative would be to return the originally loaded markup.

@aduth aduth merged commit 33542dd into master Aug 7, 2017
@aduth aduth deleted the add/catching-mr-crashy branch August 7, 2017 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Blocks Overall functionality of blocks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants