fix: bugs related to fallbackUI

[EmEsA]

Description of the change

This PR fixes some bugs related to fallbackUI

1. It was possible to pass a rendered element like this:

<ErrorBoundary level={LEVEL_WARN} fallbackUI={<FallbackComponent />}>

which would cause an error ( issue #19 ) caused by this line of code:
https://github.com/rollbar/rollbar-react/blob/main/src/error-boundary.js#L66
I think there was a misunderstanding of React.isValidElement(). It checks if a thing passed to it is an element (a result of rendering a component) not a component (function or a class with render method). <FallbackComponent /> passes React.isValidElement() but the line mentioned above tries to render it again, hence the error. What is more react doesn't like lowercase names for components, also used in line 66. I think an option of passing already rendered element doesn't bring any benefit over passing a functional component. Such an option wasn't also mentioned in the docs.

2. It was not possible to properly pass a regular react component as fallbackUI as the docs suggested.

• a functional react component which takes an object with props as its keys: const FallbackComponent = ({error, resetError}) => <div>error</div>
• a class component also wouldn't work properly
  The problem was that both those things are of a type function and would go through this line:
  https://github.com/rollbar/rollbar-react/blob/main/src/error-boundary.js#L70
  It would pass the props as positional arguments, not as a props object, resulting with a component with a regular signature ({error, resetError}) destructuring both the props as undefined.
  The generator function also mentioned in the docs worked, but that's practically the same as a functional component. The difference is that a component takes params in an object ({error, resetError}) when a generator mentioned in the docs skips the object part (error, resetError). After the changes users will also be able to use class components as fallbackUI.
  WARNING: Dropping the option of passing a generator function is a breaking change.

Type of change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Maintenance
• New release

Related issues

• Fix Fallback UI throws React error and does not render fallback UI #19

Checklists

Development

• Lint rules pass locally
• The code changed/added as part of this pull request has been covered with tests
• All tests related to the changed code pass in development

Code review

• This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
• "Ready for review" label attached to the PR and reviewers assigned
• Issue from task tracker has a link to this pull request
• Changes have been reviewed by at least one other engineer

[waltjones]

@EmEsA Thank you for the PR. This is a much needed improvement, and the PR looks great.

My only concern is removing the existing behavior without a deprecation period. I know this is a zero version package, and an early one at that. But since the implementation didn't work correctly passing a component, that means all existing users are passing the wrapper function. It's also what is in the doc and example. (Update: My mistake here. The examples and doc actually pass a function component, not the special wrapper function.)

What about detecting and handling the wrapper function, adding a deprecation warning?

[EmEsA]

What about detecting and handling the wrapper function, adding a deprecation warning?

It might not be so easy. We could check FallbackUI.length which would give use the arity of the function. It would work if a function passed by the user as fallbackUI expects both params: (error, resetError) => {}. But if someone didn't need resetError they could've written it without it: (error) => {}. In this case the length is 1, the same as for a React component.
The only way I can think of in this case is to get fallbackUI's code as a string and look for params via some regular expressions. I don't think it would be an elegant solution. What is more we would have to consider 3 cases here: regular function, arrow function and a class.

[waltjones]

I did some testing, and found that the existing implementation doesn't work with a wrapper function that returns a React Component. The intent may have been to say React Element, but the examples in the SDK doc and in https://github.com/rollbar/rollbar-react/blob/main/examples/index.js all pass a function component, not this other function signature.

Based on the above, I don't think there's a meaningful legacy case to support, and this can be merged as is.

[waltjones]

Nice work, and thanks again.

[EmEsA]

Thanks, I'm happy I could contribute :)

[waltjones]

This is now published in v0.10.0. https://github.com/rollbar/rollbar-react/releases/tag/v0.10.0