-
-
Notifications
You must be signed in to change notification settings - Fork 198
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
Provide extra props to fallback component #26
Comments
What stops you from passing an inline function component currently? const _Uploader = lazy(() => import("uploader"));
type Props = { onRequestClose: () => void };
const Uploader: SFC<Props> = props => (
<ErrorBoundary
FallbackComponent={({ error }) => (
<MyErrorComponent error={error} onRequestClose={onRequestClose} />
)}
>
<Suspense fallback={<MyLoadingComponent {...props} />}>
<_Uploader {...props} />
</Suspense>
</ErrorBoundary>
); Here's an example: |
Wouldn't that have poor performance? The fallback component reference will be changing identity with every render of Render props don't have that issue as they bypass calling |
Creating an inline function is fast. People do that in tons of places. Rendering But if you have concerns about recreating this component, you can memoize it 😄 |
My concern is not about creating functions, but rather about React remounting a component needlessly (because it's reference identity is changing)—unless I misunderstand. Thanks for the memoizing example, that makes sense. Unfortunately it's awkward because every time My concern about remounts isn't just about performance, but also any DOM state, like |
I don't think your real complaint here is with render props vs React elements. I think it's with the ability to pass arbitrary custom props along. Either way render props are called (as vanilla functions or wrapped in But as far as render props (functions) vs React elements, I think React elements are a more useful pattern because they are more flexible for users. (You can pass a function component or a class component, your component can use "hooks", etc.) They also fit into React's concurrent rendering model a bit better, since React can decide if it should process the components now or wait until the next frame. |
I think this is micro-optimizing. How expensive is this "whole new component"? It's really just a tiny function that returns a React element. It's like recreating an inline click event handler.
Why would it lead to a remount? The only thing that would remount in this case would be your error message. If that's shown, your app is in a failure state. (And also the inline message prop seems unlikely to actually change in this case.) |
That was my understanding. Re. performance, I agree! However I think the point about losing DOM state (e.g. focus on an inner button) due to remounts is more worrying, as that could be bad UX/accessibility—albeit it's quite unlikely you'll have much going on inside of an error fallback component, but ideally we wouldn't have to make that assumption. |
Just to be clear, the children you wrap inside of your error boundary- in the normal (non-errored) state– would not remount or be impacted by this. The only thing that would remount would be your fallback UI, in which case...your app is broken. It can't recover. (The I think fallback components are usually static overlays, but...assuming you had some form elements in there that were stateful– under what conditions would your |
Wouldn't the fallback component also be recreated every time If Is my understanding correct? If so, the former scenario seems not so unlikely to me. Of course it's easy to make it memoized, but is that an assumption/requirement we want to bake in to this library? (Thanks for helping me understand this!) |
Why would this matter? Again, this "component" is a very small function, not unlike a click handler. Recreating it is fast.
No problem. It's an interesting discussion. :) |
If the fallback component is recreated (fast so not a problem in itself), it will be remounted (fast so not a problem in itself). In turn, the remount would mean any DOM state of the fallback component will be lost (e.g. an inner Initially I thought recreating + remounting components needlessly was a performance issue, but the more I've given it thought—and after you clarified the situation as negligible—now my hesitation is around trying to avoid bad UX or accessibility issues. Of course there are many scenarios where this won't be an issue (most of the time, we presume the fallback component will be a simple error message). |
It's funny because I'm having a similar conversation in another repo with someone else this morning (bvaughn/react-window/issues/85) In this case of this specific library, I don't feel very strongly about any particular API decision. I didn't think this component would get much use when I created it. I thought it would just be an example to reference. If you'd like to submit a PR that changes the component to pass through props in a way similar to this, I wouldn't really object: render() {
const {children, FallbackComponent, ...rest} = this.props;
const {error, info} = this.state;
if (error !== null) {
return (
<FallbackComponent
{...rest}
componentStack={
// istanbul ignore next: Ignoring ternary; can’t reproduce missing info in test environment.
info ? info.componentStack : ''
}
error={error}
/>
);
}
return children;
} |
Sorry to interrupt. I have opened an issue on reactjs.org to seek for clarification on the render props documentation. I was told (and convinced) that render props is a better pattern because that would not trigger re-mount when inlining (see example at reactjs/react.dev#1241 (comment)). But reading your discussion makes me feel the opposite. Is there no universally better pattern? Can you shed some light on this? |
This is a very nuanced topic. As I mentioned in my comment right above, I've discussed this in depth on a recent GitHub issue. Rather than repeat it all, here you go: |
We're implementing a new <ErrorBoundary fallbackRender={props => <YourErrorFallback {...props} your="extra props" />}>
...
</ErrorBoundary> |
🎉 This issue has been resolved in version 2.0.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
I would like to pass extra props into the fallback component.
My use case:
I want my fallback component,
MyErrorComponent
, to receive the proponRequestClose
, in the same way I provide it toMyLoadingComponent
.Perhaps the fallback component should be provided as a render prop instead of as a
Component
?The text was updated successfully, but these errors were encountered: