JS Error Tracking: Allow custom error reporting logic to be called in Error Boundaries via a WP action hook #42024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +696 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
fullofcaffeine
commented
Jun 29, 2022
packages/customize-widgets/src/components/error-boundary/index.js
Outdated
Show resolved
Hide resolved
gziolo
reviewed
Jun 29, 2022
There was a problem hiding this comment.
Nice start, I think we only need to figure out how to address the concern you raised in https://github.com/WordPress/gutenberg/pull/42024/files#r909107114. Otherwise, it's all about documenting this new functionality so people can learn about it.
Any security concerns?
Great question. We will have to examine what exactly componenDidCatch exposes in error and info params.
Should we worry about multiple
addActioncalls setting multiple callbacks? (I think not)
Do you mean that the same callback can be registered multiple times if the app doesn't guard it? Can you explain what case are you worried about?
Totally optional for the first version. The fun part is that ErrorBoundry is the only React component that isn't using the functional form which became unusual in the world of hooks. I would be curious to check if we could abstract it also in a way so we don't have to use the extends syntax. There is a good article that covers it:
https://blog.logrocket.com/react-error-handling-react-error-boundary/. I'm definitely NOT advocating for using a library presented in the article, but maybe we can borrow some pattern that abstracts all that in a nice fashion.
packages/customize-widgets/src/components/error-boundary/index.js
Outdated
Show resolved
Hide resolved
gziolo
added
Developer Experience
Mamaduka
reviewed
Jun 29, 2022
packages/edit-navigation/src/components/error-boundary/index.js
Outdated
Show resolved
Hide resolved
Ah, just saw this after I replied here. Thanks for the pointer. I'm definitely interested in working on that as a follow-up! |
Actually scratch that concern. It's just that I'm not too versed in the WP hooks engine, and thought we might not want to have multiple callbacks for the same action, but thinking again, that's actually a feature and a legit use-case. |
Any group of people in the community that could help us audit this? |
|
For the first iteration, we can proceed with the unified way of calling the action that has usage documented in the reference guide. The closest page to what we need here would be Editor Filters: https://github.com/WordPress/gutenberg/blob/trunk/docs/reference-guides/filters/editor-filters.md The name is a bit confusing when I think about it, because we are dealing with the action here. However, it's a structural mistake of calling hooks - filters in the Gutenberg documentation 😞 As for the filter name, it looks like we aren't consistent with using the name of the package as the prefix so we can go with the general doAction( 'editor.ErrorBoundary.errorLogged', error, errorInfo ); |
…dditional error reporting logic for Error Boundaries
Separate navigation editor screen development is on hold.
fullofcaffeine
force-pushed
the
add/error-reporting-action-hooks-to-error-boundaries
branch
from
b69f546 to
32671b5
Compare
fullofcaffeine
commented
Jul 8, 2022
| @@ -0,0 +1,38 @@ | |||
| /** | |||
There was a problem hiding this comment.
The unit-tests are identical but simple enough, but there's one for each error boundary, which is a symptom of the existing unDRYness for Error Boundaries. We can turn them into a single test once we work on #42024 (comment).
There was a problem hiding this comment.
Since we plan to work on #42024 (comment) soon, I'll abstain from trying to DRY the unit-tests (by extracting the common logic into a shared function or something along those lines, for example).
fullofcaffeine
requested review from
noisysocks,
draganescu,
talldan,
tellthemachines,
adamziel,
kevin940726,
ajitbohra,
ryanwelcher and
juanmaguitar
as code owners
gziolo
approved these changes
Jul 12, 2022
There was a problem hiding this comment.
This is a great start. Thank you @fullofcaffeine for bringing this task to the finish line.
The new documentation page is disconnected from the Block Editor Handbook. I will take care of changes to the Reference Guides separately as it requires some discussion how to rephrase all sections for filters to cover also actions similar to what you can see in WordPress core.
gziolo
added
the
Needs Dev Note
|
I opened a follow-up that tries to re-structure documentation to better surface the new error logging capability for extenders. I also marked this PR with the |
|
I added |
fullofcaffeine
added a commit
to Automattic/wp-calypso
that referenced
this pull request
Jul 12, 2022
… in Gutenberg with Sentry Register a callback to `editor.ErrorBoundary.errorLogged` action that will forward the error to Sentry everytime the action is called. The action has been added to all active React Error Boundaries in Gutenberg in the following changeset: WordPress/gutenberg#42024 .
fullofcaffeine
added a commit
to Automattic/wp-calypso
that referenced
this pull request
Aug 5, 2022
… in Gutenberg with Sentry Register a callback to `editor.ErrorBoundary.errorLogged` action that will forward the error to Sentry everytime the action is called. The action has been added to all active React Error Boundaries in Gutenberg in the following changeset: WordPress/gutenberg#42024 .
fullofcaffeine
added a commit
to Automattic/wp-calypso
that referenced
this pull request
Aug 5, 2022
… in Gutenberg with Sentry (#65503) Register a callback to `editor.ErrorBoundary.errorLogged` action that will forward the error to Sentry everytime the action is called. The action has been added to all active React Error Boundaries in Gutenberg in the following changeset: WordPress/gutenberg#42024 .
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Create an extension point in Gutenberg Error Boundaries that allows any client code (browser JS code running in the same context as the GB editor) to call custom logic as part of the boundaries'
componentDidCatchmethods, via a WP action hook. This logic can be set by any JS code running in the same context as the Gutenberg plugin that has access to import and useaddActionfrom the@wordpress/hookspackage.Why?
Error Boundaries provide a great way to customize the way rendering errors are caught and presented to the user, however, they end up swallowing the errors. This means that if you want to report it somewhere else, you're out of luck (as in, you need to modify the actual GB source code).
A typical real-world scenario is the use of tools like Sentry or Rollbar. By default, these tools setup a global error handler that will track and capture any uncaught error that ends up bubbling up to it. This is not the case for errors handled by React Error Boundaries, so they end up not being captured at all.
These tools usually provide their own Error Boundaries that will take care of capturing the errors, but Gutenberg already has many Error Boundaries with their custom logic, and we're not looking to replace them. We also don't want to couple the custom error reporting logic to the Error Boundary classes, either. We want them to be optional and set by any other JS code running in the same context (any other plugin/mu-plugin or enqueued js file).
How?
For each React Error Boundary in Gutenberg, as part of the
componentDidCallcallback, we introduce a newgb.reportErrorBoundaryExceptionaction hook call (the name is provisory and up for discussion) and pass theerroras its payload:Then, in any client JS code - which could be, say, the logic that sets up a tool like Sentry or Rollbar - we can then do something like:
TODO
addActioncalls setting multiple callbacks? (I think not)Testing Instructions
1) In your WP test instance, add a
wp-content/mu-plugins/test.phpfile with the following contents:2) Force an error in a component that's wrapped by one of the available React Error Boundaries. Here's a diff for the
PostTitlecomponent, which will test the Error Boundary inpackages/editor/src/components/error-boundary:Apply this diff to your custom build of GB based on this branch. Make sure it's installed and active in your WP test instance.
3) Start a new post. You'll see the
The editor has encountered an unexpected error.warning message, and if you open the console, you should see theerrorlog prefixed withOpsie, meaning the custom action callback you setup in step 1) ran successfully.Resolves: #41094