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

firestoreConnect permission exceptions are uncaught #386

Closed
lukeda opened this issue Jan 29, 2018 · 7 comments
Closed

firestoreConnect permission exceptions are uncaught #386

lukeda opened this issue Jan 29, 2018 · 7 comments

Comments

@lukeda
Copy link

lukeda commented Jan 29, 2018

Do you want to request a feature or report a bug?

Feature request

What is the current behavior?

When initialising firestoreConnect at the App component level, if permissions do not allow one of the collecitons to be queried, an exception is thrown.

Uncaught Error in onSnapshot: Error: Missing or insufficient permissions.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via codesandbox or similar.

Component:

firestoreConnect(['todos'])

Firestore Rules:

match /todos/{todoId} {
allow read, write: if false;
}

What is the expected behavior?

It would be nice if we could allow the app to gracefully handle this exception and display a warning to the user.

Which versions of dependencies, and which browser and OS are affected by this issue? Did this work in previous versions or setups?

react-redux-firebase@2.0.2

@prescottprue
Copy link
Owner

@lukeda Are you seeing anything written into state.firestore.errors? Though an error is being thrown the listener should hopefully still update state.

Not sure we want to silence the Error itself since the Firebase methods themselves throw, but it should be helpful to see the error appear in state.

@lukeda
Copy link
Author

lukeda commented Jan 30, 2018

I am currently not seeing any errors in the firestore state. It might be a bit ambitious but I would love to be in control of everything my app specifically outputs to the console. Perhaps a 2nd parameter to firebaseConnect which catches the exception and lets the code deal with it. If that 2nd parameter isn't present then continue to throw the error?

Console output:
https://i.imgur.com/3BodEGE.png

Not sure if this is a good place to mention the status object never seems to update for firestore. I have ended up writing my own dataLoaded state to detect dispatches from SET_LISTENER and LISTENER_RESPONSE. I may have missed another helper reducer.

@prescottprue
Copy link
Owner

prescottprue commented Feb 6, 2018

setListener, which is used internally in firestoreConnect, already has support for an error callback, so hopefully we can make use of that.

I like the idea of passing another argument to firestoreConenct, but it may take a little thinking to get the API right.

As for the state not updating, another person reported, and I was able to replicate it now.

Its seems to be due to a small issue within the errorsReducer for which I am working on a fix. Thanks for reporting.

@prescottprue
Copy link
Owner

prescottprue commented Feb 6, 2018

Once the fix is in place the intent will be to get errors from the errors state by the dot query path.

Here is a little snippet using recompose's branch and renderComponent:

import React from 'react'
import { compose } from 'redux'
import { connect } from 'react-redux'
import { branch, renderComponent } from recompose

const ErrorComponent = ({ todosError }) => (
  <div>Error Loading Todos: {todosError.code || 'Error'}</div>
)

const enhance = compose(
  firestoreConnect(['todos']),
  connect((state) => ({
    todos: state.firestore.data.todos,
    todosError: state.firestore.errors.byQuery.todos
  }),
  branch(({ todosError }) => !!todosErrors, renderComponent(ErrorComponent))
)

For now it doesn't handle multiple errors at one query path well, but that can come in the future.

This also makes me thing we could include selectors and HOCs for accessing this through a more clean API.

@compojoom
Copy link
Contributor

@prescottprue - when I read the description "fix errors reducer to correctly update on Listener_error" I was under the impression that if a listener encounters an error, it will be able to recover from this error and fetch the data at a later point? Or is this not intended?

I have the following situation. When the user logs in - I'm listening to a document that is supposed to be accessible just by that 1 user. (there is a rule in place) However this document is created with a cloud function and it can take some time to create the document. So once the user logs in I get the error that the user doesn't have access to that path. Now the only way to show the data at that path once it is available is to restart the app. Is there a way around this?

@prescottprue
Copy link
Owner

@compojoom Yes you can "recover" in the sense that the error should be written to state (with the error logged and not thrown). That way the error at that path will still be there.

This PR on redux-firestore is where I am currently working on fixing the errorsReducer (which is not currently updating state with the error value as intended). What you are describing should work great after that is merged.

prescottprue added a commit to prescottprue/redux-firestore that referenced this issue Feb 9, 2018
* fix(errorsReducer): correctly update on `LISTENER_ERROR` dispatch - [RRF #386](prescottprue/react-redux-firebase#386)
* feat(tests): more unit tests added for `orderedReducer` (including one covering update of existing document within subcollection)
* fix(types): typescript typings updated (non existent constants removed)
@prescottprue
Copy link
Owner

v0.2.8 release of redux-firestore includes a fix for the errorsReducer. That means things should update in a way that the above example should work.

Reach out if there are any more questions.

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

No branches or pull requests

3 participants