-
Notifications
You must be signed in to change notification settings - Fork 22
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
Pass along errors from firebase #46
Conversation
I'm not sure how to handle errors of large amount of refs. const App = props => {
if (
props.one instanceof Error
props.two instanceof Error ||
props.three instanceof Error ||
props.four instanceof Error ||
props.five instanceof Error ||
) {
return <div>🚫</div>
}
return <div>✅</div>
}
connect({
one: 'one',
two: 'two',
three: 'three',
four: 'four',
five: 'five',
})(app) Maybe this is fine and introduce a more general onError handling if theres a need for it later. |
@einarlove It's IMHO more flexible being able to define what should be passed to the component, either the first error (fatal), or every error in as an array or as an object with a field mapping. |
So, you're for #44 instead since it deals with errors in |
@einarlove This might be a bit less user-friendly but it's a lot more flexible being able to handle this in user-land. E.g. Return null on error if a field isn't critical. So no, I like this approach more |
If we merge this, what semver release should this be? No change in API, but the behaviour has changed if you get an error. Today it's just silent and the prop is null, if this is merged, you'll get a React error if you try to render the Error. |
@einarlove While I'm not completely sure, I think breaking the most central contract of the existing API warrants a major bump. |
Maybe we could add option to catch all errors like: const onFirebaseError = error => {
error.code // PERMISSION_DENIED
error.message // permission_denied at /forbidden: Client doesn't have permission to access the desired data.
if (error.code === 'NETWORK_ERROR') {
showNoInternetMessage()
}
if (error.code === 'PERMISSION_DENIED') {
logoutUser()
}
}
<Provider firebaseApp={firebaseApp} onError={onFirebaseError}>
<App />
</Provider> and silence the errors below with: const onError = error => {
if (!this.mounted) return
if (this.context.firebaseAppErrorHandler) {
this.context.firebaseAppErrorHandler(error)
return
}
… Then again, I don't know if this will be blasted with 30 errors when no network/auth and how to handle that. We need to test this feature more if we are to merge this. |
Capture errors from firebase and pass them along to connected component.
Example:
Resolves #43