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

Pass along errors from firebase #46

Closed
wants to merge 2 commits into from
Closed

Pass along errors from firebase #46

wants to merge 2 commits into from

Conversation

einarlove
Copy link
Member

@einarlove einarlove commented Jun 23, 2017

Capture errors from firebase and pass them along to connected component.

Example:

const App = ({ data }) => {
  if (data instanceof Error) return (
    <div>Oh no! {data.message}</div>
  )
  
  return (
    <div>
      {JSON.stringify(data)}
    </div>
  )
}

connect({
  data: 'path-to-data',
})(App)

Resolves #43

@einarlove
Copy link
Member Author

einarlove commented Jun 23, 2017

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.

@simenbrekken
Copy link
Contributor

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

@einarlove
Copy link
Member Author

So, you're for #44 instead since it deals with errors in mapSubscriptionsToProps?

@simenbrekken
Copy link
Contributor

@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

@einarlove
Copy link
Member Author

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.

@simenbrekken
Copy link
Contributor

@einarlove While I'm not completely sure, I think breaking the most central contract of the existing API warrants a major bump.

@einarlove
Copy link
Member Author

einarlove commented Jun 26, 2017

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.

@simenbrekken
Copy link
Contributor

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.

None yet

2 participants