prescottprue / react-redux-firebase

Redux bindings for Firebase. Includes React Hooks and Higher Order Components.
https://react-redux-firebase.com
MIT License
2.55k stars 559 forks source link

feat(query): easier access to firestoreConnect errors in components #703

Open peteruithoven opened 5 years ago

peteruithoven commented 5 years ago

Feature request: Easier access to errors from firestoreConnect (reading data) in components.

What is the current behavior? When a error arises when requesting data the error information is stored in the Redux state at firestore.errors.byQuery.[query id]. A query id could simply be the name of a collection like todos, but when specifying a limit it becomes todos?limit=6 making it hard to read out and forward to a component.

What is the expected behavior? I'd love an easier way to pass that those errors as a prop to a component.

I figured out a work around using storeAs:

import React from 'react';
import { connect } from 'react-redux';
import { firestoreConnect, isLoaded, isEmpty } from 'react-redux-firebase';
import { compose } from 'redux';

function List({ todos, error }) {
  if (!isLoaded(data)) return 'Loading...';
  if (isEmpty(data)) return null;
  if (error) return error.message;
  return (
    <ul>
      {todos.map(todo => (
        <li key={todo.id}>{todo.name}</li>
      ))}
    </ul>
  );
}

const QUERY_ID = 'todos';
export default compose(
  firestoreConnect([
    {
      collection: 'todos',
      storeAs: QUERY_ID,
    },
  ]),
  connect(state => ({
    [QUERY_ID]: state.firestore.ordered[QUERY_ID],
    error: state.firestore.errors.byQuery[QUERY_ID],
  }))
)(List);

I'm using:

illuminist commented 5 years ago

redux-firestore has internal utility function called getQueryName which is being used for determining and creating query name from query object input into firestoreConnect.

It hasn't been officially exported, so to use this you can import it by

import { getQueryName } from 'redux-firestore/es/utils/query'

Example usage:

const makeQuery(visibility) => ({
  collection: 'todos',
  where: ['visibility', '==', visibility],
})
export default compose(
  firestoreConnect(({visibility}) => [makeQuery(visibility)]),
  connect((state, {visibility}) => ({
    todos: state.firestore.ordered.todos,
    error: state.firestore.errors.byQuery[getQueryName(makeQuery(visibility))],
  }))
)(List);

I think it's a good idea to export getQueryName or some way to return a query name from the connector itself.

peteruithoven commented 5 years ago

Thanks for the reply! Making getQueryName seems like a great improvement.

I see you've created the new React hooks, like useFirestoreConnect(), maybe those can make things easier. We probably don't want to retrieve the error directly from useFirestoreConnect() since, like the data it should probably always be connected to that data in redux. But maybe it could return the query name it's connected to? One downside is that it might be confused with the path to use to retrieve data, which can be different (example: data is retrieved from state.firestore.ordered.todos while the error is at state.firestore.errors.byQuery.todos?visibility=true).

That would result in something like:

import React from 'react';
import { useSelector } from 'react-redux'
import { useFirestoreConnect, isLoaded, isEmpty } from 'react-redux-firebase';

export default function List() {
  const { queryName } = useFirestoreConnect({
    collection: 'todos',
    where: ['visibility', '==', visibility],
  });
  const todos = useSelector(state => state.firestore.ordered.todos)
  const error = useSelector(state => state.firestore.errors.byQuery[queryName])
  if (!isLoaded(data)) return 'Loading...';
  if (isEmpty(data)) return null;
  if (error) return error.message;
  return (
    <ul>
      {todos.map(todo => (
        <li key={todo.id}>{todo.name}</li>
      ))}
    </ul>
  );
}

Another approach might be to (also) return redux paths from useFirestoreConnect, but that requires functions. I don't have enough experience with hooks to know if this is a good idea.

import React from 'react';
import { useSelector } from 'react-redux'
import { useFirestoreConnect, isLoaded, isEmpty } from 'react-redux-firebase';

export default function List() {
  const { getOrdered, getError } = useFirestoreConnect({
    collection: 'todos',
    where: ['visibility', '==', visibility],
  });
  const todos = useSelector(state => getOrdered(state))
  const error = useSelector(state => getError(state))
  if (!isLoaded(data)) return 'Loading...';
  if (isEmpty(data)) return null;
  if (error) return error.message;
  return (
    <ul>
      {todos.map(todo => (
        <li key={todo.id}>{todo.name}</li>
      ))}
    </ul>
  );
}

For comparison, using getQueryName util, which only seems slightly harder:

import { useSelector } from 'react-redux'
import { useFirestoreConnect, isLoaded, isEmpty } from 'react-redux-firebase';
import { getQueryName } from 'redux-firestore/es/utils/query'

export default function List({ visibility }) {
  const query = {
    collection: 'todos',
    where: ['visibility', '==', visibility],
  };
  useFirestoreConnect(query);
  const todos = useSelector(state => state.firestore.ordered.todos)
  const error = useSelector(state => state.firestore.errors.byQuery[getQueryName(query)])
  if (!isLoaded(data)) return 'Loading...';
  if (isEmpty(data)) return null;
  if (error) return error.message;
  return (
    <ul>
      {todos.map(todo => (
        <li key={todo.id}>{todo.name}</li>
      ))}
    </ul>
  );
}
illuminist commented 5 years ago

Thank you! I like the second idea where a hook returns state selectors for its own query. That should make life much easier for everyone.

I've been thinking about making pagination easier which I planned to take the same approach, which should include { readNext, readPrevious, readAppend }, but I still need to research and refactor redux-firestore internal to be able to do that first.

prescottprue commented 5 years ago

Love all of this discussion and input! We may want to expose getQueryName at the top level

mariotacke commented 4 years ago

I'd like to capture any FirebaseError: Missing or insufficient permissions. when using useFirestoreConnect. Is this currently possible? My use case is "trying to access documents you shouldn't". I'd like to capture this "403 - Forbidden" scenario somehow and redirect the user.

prescottprue commented 4 years ago

@peteruithoven I really like the idea of hooks returning functions to help load from state - this will be particularly useful for v1 of redux-firestore since it stores things in state under the query path.

@mariotacke did you check to see if that is written to state at all. The above discussion is about loading those errors out of state in the easiest way

mariotacke commented 4 years ago

@prescottprue, yeah I've noticed there is an error object on the state that records these things. I used that one. Thanks for your help!

illuminist commented 4 years ago

@prescottprue More input from me, it'd be better if we have a helper to create selector function instead.

For example

const { errorSelector, dataSelector, orderedSelector } = makeFirestoreSelector(query)

So the accessing to state isn't exclusive to hooks and should be able to be used in firestoreConnect and redux connect HOC. And it even has potential to combine data and ordered state selector to denormalize data if we're going to change ordered state to store only id.

prescottprue commented 4 years ago

@illuminist That is a really great point and I like the proposed API - thanks for sharing!