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

fix: bugs related to fallbackUI #29

Merged
merged 1 commit into from
Feb 16, 2022

Conversation

EmEsA
Copy link
Contributor

@EmEsA EmEsA commented Jan 28, 2022

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.

  1. 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

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

simplify the solution

update examples and readme
@waltjones
Copy link
Contributor

waltjones commented Feb 11, 2022

@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
Copy link
Contributor Author

EmEsA commented Feb 16, 2022

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
Copy link
Contributor

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.

Copy link
Contributor

@waltjones waltjones left a comment

Choose a reason for hiding this comment

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

Nice work, and thanks again.

@waltjones waltjones merged commit 2ab890c into rollbar:main Feb 16, 2022
@EmEsA
Copy link
Contributor Author

EmEsA commented Feb 17, 2022

Thanks, I'm happy I could contribute :)

@waltjones
Copy link
Contributor

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

ijsnow pushed a commit to ijsnow/rollbar-react that referenced this pull request Mar 7, 2022
simplify the solution

update examples and readme
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.

Fallback UI throws React error and does not render fallback UI
2 participants