Building off of my other PR: Passing error to children #9
Conversation
Using div means this affects the dom. This could have unintended consequences. Replacing it with React.Fragment (<> shorthand) removes that issue.
|
Those are good improvments for this package:) Thank you:3 |
| @@ -64,7 +64,7 @@ export function _render (self, ProvideContext) { | |||
| const __html = renderToStaticMarkup(elementWithProviders) | |||
| return <div dangerouslySetInnerHTML={{__html}} /> | |||
| } catch (e) { | |||
| return <div>{self.props.fallBack()}</div> | |||
| return <>{self.props.fallBack()}</> | |||
There was a problem hiding this comment.
I think, the Fragmet syntax is excess here:) fallBack'll return some jsx elements already
There was a problem hiding this comment.
If the server's fallback gets an exception's error in arguments, it would be cool too;)
...
catch (error) {
return self.props.fallBack({ error })
}
There was a problem hiding this comment.
This is going to be a problem when React tries to hydrate the DOM. If the DOM structure doesn't match what would normally be rendered, then it can result in zombie elements which do not get re-rendered. The fallback content isn't going to be the same as whatever we expected to render, but removing that div even with no fallback will be something that hydrate handles badly.
| const { error, errorInfo, hasError } = this.state; | ||
| if (hasError) { | ||
| return this.props.fallBack({ error, errorInfo }); | ||
| } else { |
There was a problem hiding this comment.
You can write conditions after "else" without "else". Possible, it'll be read better that way;)
const { fallBack, children } = this.props
if (hasError) return fallBack({ error, errorInfo })
return typeof children === 'function' ? children({ error, errorInfo }) : children
|
@krishnaglick could you publish a release on your own github repo? I would like to start using your version instead 🙂 |
|
This didn't end up working exactly how I wanted, and I haven't had time to revisit it. |
I didn't want to redo all the footwork here: #8
So that should be looked at and merged first.