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

Feature/error boundaries #151

Merged
merged 25 commits into from
May 12, 2021
Merged

Feature/error boundaries #151

merged 25 commits into from
May 12, 2021

Conversation

nicolai-nordic
Copy link
Contributor

@nicolai-nordic nicolai-nordic commented May 10, 2021

This PR implements https://trello.com/c/By7DaokJ/40-make-initial-implementation-of-react-error-boundaries.

In this PR we add a Error Boundary to shared. The purpose of this is that whenever an error occurs in a React component within the Error Boundary, the Error Boundary will catch the error and we can display a descriptive error message and provide the user with tools for recovering. Until this was implemented the users were only shown a blank screen when an error occured within the React tree.

image

We will also send a GA event whenever an error occurs reporting the application, platform, architecture, app version and error message header, which will provide us information about the frequency of these errors.

This PR will only provide Error Boundaries for newer apps with the latest architecture, but as soon as this is merged I will create a PR implementing the same for legacy apps.

The latest update to this PR added an initial test for the Error Boundary. The reason for adding the following to the test

const spy = jest.spyOn(console, 'error');
spy.mockImplementation(() => {});
spy.mockRestore();

is described here.
TL;DR throwing an error inside a React component will output the error to the console, even if it is caught by a Error Boundary. So we suppress this message to not pollute the console output.

nicolai-nordic and others added 4 commits May 10, 2021 14:09
The change not only adds the component itself, but also uses it to
increase error robustness to all apps. So it is nice to mention that
also.
The property `children` is by default already included in
React.Component, so one usually does not need to specify it.

You _can_ specify it though, if you want it to have a more specific or
different type than the default. Here it was actually made mandatory,
but that seems to be unintended, so I removed it.
@datenreisender
Copy link
Contributor

In the branch feature/error-boundaries_enhancements I collected some change suggestions. Pick from it what you like.

There are some aspects, where I am not so sure whether you should change them, because I am uncertain about your motivation or they may be more a matter of different opinions:

  • Why did you add getInitializedStatus to src/utils/usageData.ts instead of directly using the already exported property isInitialized? In any case, it does not seem necessary, to make getInitializedStatus an async function.
  • In the About pane we generate a system report file when users request it. In the error dialog you instead just create and display the text from the system report. Why don't you instead also create a file on request? I think the latter would make it easier for users to subsequently also send this file to us (through mail, in a GH issue or on DevZone).
  • The App component receives a property reportUsageData which apps can use to signal whether they want to report usage data. Your implementation ignores this and always initialises Google Analytics and sends an error report through it. Is that intentional or an oversight? I think if third parties develop their own apps, we should leave it open to them, whether they want their apps to also send usage data and errors to us.
  • Why do you in
    const isInitialized = await isGAInitialized();
    if (!isInitialized) {
    await initGA(packageJson());
    }
    first check whether GA is initialised and if not then do initialise it? Because GA may not be initialised yet? Or because the app may not say that it wants to report usage data to GA? Usually, I think it should not be necessary to initialise GA in this component. And I would like to keep error boundary component as simple as possible, to minimise that we have errors in it which in turn would not be caught by the error boundary itself.
  • Making componentDidCatch an async function seems wrong: The returned Promise is not waited upon by React, so it just means that it will return even before the state is updated. And we also do not need to wait until the GA event is sent, that should happen in the background anyway.
  • Did you take a look at getDerivedStateFromError? According to the note at the end calling setState will be deprecated in componentDidCatch in the future.
  • Did you try whether also correctly resets the launcher store when the component is used in the launcher? Because it looks to me, like this only resets the store of an app (which is why you might also want to keep the name getAppSpecificStore instead of renaming it).

You do not have to answer these points. Feel free to either ignore them or discuss them with me in synchronously instead of asynchronously.

@nicolai-nordic
Copy link
Contributor Author

Lots of great feedback, thanks!

  • I created this helper because I had some issues with the isInitialized property being set to false even when it should not have been when the app crashed in very early stages.. I can revisit this issue to confirm.
  • I didn't realize this was something we wanted, but we can of course provide this functionality. Should it be a button like we do in the About pane instead of the accordion which is there now or should we provide both?
  • It was both intentional and an oversight. I did not consider any third-party apps, which is a fair point, but I'm not sure if anyone actually has done this? The main reason I am initializing the GA events regardless of the reportUsageData flag being passed to App is that I noticed that if the app crashed early it had not yet initialized GA, which caused us to not send events. Again this is something I can verify. Also this gives us error events from apps that currently do not have the reportUsageData, such as the BLE app. Of course we can just add this flag to that app.
  • Good point on the async componentDidCatch.
  • I did not check that out, but I will.
  • This is currently not implemented for the launcher, and I don't know yet how we want to solve this, so I was postponing it for the future.

datenreisender
datenreisender previously approved these changes May 11, 2021
@nicolai-nordic nicolai-nordic merged commit 0509b2e into master May 12, 2021
@nicolai-nordic nicolai-nordic deleted the feature/error-boundaries branch May 12, 2021 08:27
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.

3 participants