search

rollbar / rollbar-react

React features to enhance using Rollbar.js in React Applications
https://docs.rollbar.com/docs/react
MIT License
42 stars 10 forks source link

fix: bugs related to fallbackUI #29

Closed EmEsA closed 2 years ago

EmEsA commented 2 years ago

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

Related issues

Checklists

Development

Code review

waltjones commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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.

EmEsA commented 2 years ago

Thanks, I'm happy I could contribute :)

waltjones commented 2 years ago

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