feat(reacthook) React hook API

[illuminist]

Description

I introduced new 4 apis that use react hook.

• useFirebase
• useFirebaseConnect
• useFirestore
• useFirestoreConnect

useFirebase and useFirestore

Basically just returning firebase and firestore instance accordingly. Internally uses useContext hook to get the instance from ReactReduxFirebaseContext

useFirebaseConnect and useFirestoreConnect

Almost exactly same as firebaseConnect and firestoreConnect HOC, except:

• to conform with "rules of hook", each hook now accepts only one query, no more array. This simplified implementation greatly and make hook much simpler than the original one.
  The reason of this is useEffect hook has to memoize data dependency. If there are multiple queries in a hook. useEffect will clean up(unset) all the queries in it even only one query has been changed, leading to unnecessary unset/set listener.
  And we cannot use multiple useEffect hook on an array of query. "Rules of hook" state that hook should not be use in a loop or conditionally assign.
• If a query is an object(usually firestoreConnect query), the query needed to be memoized before being applied to useFirestoreConnect. If different query object, even one with exactly same value, will cause hook to unset/set listener.

Example

const Component = ({ todoList }) => {
  useFirebaseConnect('todos') // use just like firebaseConnect, string query isn't need to be memoize
  return <div>{JSON.stringify(todoList)}</div>
}
export default  connect(state => ({ todoList: state.firebase.data.todos }))(Component)

const Component = ({ todoData, todoId }) => {
  useFirebaseConnect(`todos/${todoId}`) // string query isn't need to be memoize
  return <div>{JSON.stringify(todoData)}</div>
}
export default  connect((state, {todoId}) => ({ todoList: state.firebase.data.todos[todoId] }))(Component)

const Component = ({ todoData, todoId }) => {
  const query = useMemo(() => ({ path: `todos/${todoId}` }), [todoId])
  useFirebaseConnect(query) // Query object need to be memoize first
  return <div>{JSON.stringify(todoData)}</div>
}
export default  connect((state, {todoId}) => ({ todoData: state.firebase.data.todos[todoId] }))(Component)

Note: useFirebaseConnect and useFirestoreConnect will not accept array of query anymore
To listen to multiple queries, use multiple connector hooks

const Component = ({ userId, userData, todoList }) => {
  useFirebaseConnect('todos')
  useFirebaseConnect(`users/${userId}`)
  return <div>
    <div>{JSON.stringify(todoList)}<div>
    <div>{JSON.stringify(userData)}</div>
  </div>
}
export default  connect(state => ({
  todoList: state.firebase.data.todos,
  userData: state.firebase.data.users[userId] 
}))(Component)

Dependency

react version needed to be upgrade to 16.8 which provide hook API

Check List

• All tests passing
• Docs updated with any changes or examples if applicable
• Added tests to ensure new feature(s) work properly

Relevant Issues

[codecov]

Codecov Report

Merging #684 into next will increase coverage by 0.73%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##             next     #684      +/-   ##
==========================================
+ Coverage   82.25%   82.98%   +0.73%     
==========================================
  Files          26       30       +4     
  Lines         924      964      +40     
  Branches      166      168       +2     
==========================================
+ Hits          760      800      +40     
  Misses        164      164

[prescottprue]

I was starting to write these myself, but didn't finish, great work!

[samsoul16]

As all the checks have passed and you also seem in conjunction with them, i would really request you @prescottprue to please merge them to the next branch i am really waiting for this, as it will remove the use of firestoreConnect completely and we can move on to more newer hook's culture introduced by React.

The below question i'm not sure whether it belongs here but i will ask anyway as it's a suggestion, please feel free to tell me to start a new thread / issue for it instead.
Also i noticed one major thing which bugged me, which was that i didn't want to send dispatch as props ( dispatch : store.dispatch) because I'm using the latest react-redux 7.0.alpha.4 which has useDispatch Hook and i want to implement my project majorly based on Hooks. But it threw and error stating that dispatch was not defined and internally firebase was using it, and i know why that is because for internal calls you would be using it.
If we are moving to a more Hook culture will using the new useDispatch feature be a good idea? it's in alpha i realize but it's still stable and powerful enough also we also have a next branch for same things.. Refer : https://react-redux.js.org/next/api/hooks Also if not now but before you shift next to master this would be a great point to consider i feel.

[prescottprue]

@samsoul16 Thanks for the reminder! Was actually starting to look into some stuff for next, so we can for sure get this into next.

Also glad you are bringing up the useDispatch and a new issue would be awesome 😄 . Input like this is always welcome, especially with stuff around new API changes 👍

[samsoul16]

Hello @prescottprue, This can be a silly question.
After you merged this with the next branch yesterday i tried to install it using npm install react-redux-firebase to get the useFirestoreConnect Hooks. but i didn't get the new code. I'm not much knowledgeable about npm packages but do you have to push it to the npm code base before we can pull it? If yes, will i have to wait for long before we get this via npm or this can be get via any alpha? Eg. npm install react-redux-firebase@alpha.xxx something. Also if there is any other way i can use this code directly can you let me know / redirect me to somewhere where i can do it please?

It seems I'm still on alpha 11

sam16@sam16 MINGW64 /e/mindseed/odin (master)
$ npm list react-redux-firebase
odin@0.1.0 E:\mindseed\odin
`-- react-redux-firebase@3.0.0-alpha.11

which is in conjunction with https://www.npmjs.com/package/react-redux-firebase this i guess.

[samsoul16]

Hello @prescottprue , first of all i would like to really thank you for speeding up things so quick and pushing this so i could get this from npm. Also i would like to thank @illuminist for this amazing idea and work.

But now I'm stuck at a situation and would like to use some help.
So useFirestore Hook is meant to replace the withFirestore and it worked perfectly fine, but where I'm stuck is that i want to do useFirestore in my Actions file where basically it gets intercepted from thunk. Currently whenever i do const firestore = useFirestore() there i get an error saying that Hooks can be called only from a functional component so the current solution in my mind is to pass the firestore object as parameter to the action/actionCreator manually, but this is same as passing the whole props object to it so I couldn't benefit from it completely, also then this defeats the whole purpose.

I'm sharing my codepens below.

You can only look at the files action.js & Todos.js you don't need to go through other things.
React Redux Firestore With Hooks

In this codepen i haven't passed the firestore object to actions but i did it in my real project as that was the only way to get it working.

Just in case, this is the working version where i used to pass props which had firestore object when i was using withFirestore. React Redux Firestore with firestoreConnect HOC

If you guys want i can also share the code here.

[illuminist]

@samsoul16 Imagine a value from a hook is same as value get from HOC props. With this you can do the exactly same way as your HOC example, https://codesandbox.io/s/o5z54q5829. Notice that instead of props for the first parameter of action creator, I selectively input only firestore as a prop, but it's called hook dependency. With this you can use same action creator as HOC example without any modification which is a main idea for me to create these hooks.

Also notice that I also use useCallback for the function. It memoizes the function itself without creating a new function every times that causes unnecessary re-rendering when the component itself re-rendered.

I noticed that your hook example still has withFirestore HOC and calling firestore instance referencing from nowhere inside the action creator, I wonder if there's any reason for these.

Also instead of connect HOC you can use useSelector hook from react-redux instead if you wish to go 100% react hook app.

[samsoul16]

@samsoul16 Imagine a value from a hook is same as value get from HOC props. With this you can do the exactly same way as your HOC example, https://codesandbox.io/s/o5z54q5829. Notice that instead of props for the first parameter of action creator, I selectively input only firestore as a prop, but it's called hook dependency. With this you can use same action creator as HOC example without any modification which is a main idea for me to create these hooks.

Also notice that I also use useCallback for the function. It memoizes the function itself without creating a new function every times that causes unnecessary re-rendering when the component itself re-rendered.

I noticed that your hook example still has withFirestore HOC and calling firestore instance referencing from nowhere inside the action creator, I wonder if there's any reason for these.

Also instead of connect HOC you can use useSelector hook from react-redux instead if you wish to go 100% react hook app.

Hello @illuminist, first of all thank you so much for your time in explaining everything and going to the extend of forking my code-pen and correcting it for helping me out.

Here is my modified Code Pen : https://codesandbox.io/s/8423o1m529

I learned a lot from your answer and would like to point things out.

1. I'm sorry, that withFirestore was left by mistake in my actual code i had removed the withFirestore HOC.

2. Ty for reminding me of useSelector Hook, i had read it but not used it hence i didn't remember that it actually did the work of connect. Now i have removed connect and used useSelector to move to a Full Hook Based Implementation

3. That useCallback Hook is a great feature i was unknown about as i have recently started working in React and didn't actually go through everything, modified my actual code-base as well code-pen to use the same. It would really improve performance, and now because of this i learnt useMemo Hook also.

4. I specifically want to point out your usage of { firestore } instead of just firestore in addTodo({ firestore }, todo)) because then my Action Creator always have the same function declaration actionName(props, data) for every Action Creator and i can use a good coding style rule here, thanks to you. Previously i was passing different parameters to different action creators and i release it wasn't a good extensible strategy. Eg of what i was doing :
   Actions file : auth.js
   
   Component File : SignUp.js
   

Question : The Key thing here is that even tough now if props are not needed by some actionCreator i have to still pass an empty map {} to be used as props, according to this strategy, which could lead to an overhead. Do you also think so?
My proposal : We could Keep the props as second param instead and initialize it to empty map {} then if we don't need any extra props to passed to actionCreator we could just pass the data.
Eg: In Component

1. If want to pass all props
dispatch(actionCreatorName(data, props))
2. If want to pass specific props
dispatch(actionCreatorName(data, { firestore }))
3. If want to pass no props
dispatch(actionCreatorName(data))

In ActionCreator
export const actionCreatorName = (data, props = {}) => { .... }
@prescottprue would need your input here, please as we could come up with a good design pattern here

5. I also, noticed that now props will no longer have any connected values from state as i will be assigning them to directly to constants using useSelector Hook, hence now props will only have the necessary props passed to them via Parent Component. A good use case for this i could come up was let's say i want to have a separate page for showing more details for a single todo, i could just pass todo-id as prop and then in useSelector i could just filter using that todo-id.
   Eg; const todo = useSelector(state => state.todos[props.todo-id], [props.todo-id])

Thank You for your time and contributions @illuminist & @prescottprue

[illuminist]

@samsoul16 Answer on 4. Creating an empty object has negligible overhead. Let's say almost of js's immutability relies on creating a new object every times a function run. If that concerns you, you can pass null object instead which has much lower overhead. Or you can rearrange action creator parameter to have necessary data as first parameter and additional props as second parameter. But if you decorate it with useCallback hook, it will run only once or fewer times depend on which dependency provided to the hook.

Note on 5. The latest commit from react-redux removes deps array from useSelector. So beware that you will need to separately memoize a selector if you upgrade the latest react-redux

Feel free to ask if you have more questions.

[samsoul16]

@illuminist Thanks for the prompt reply.

For 4. I think i might have not stated it correctly and I'm sorry for that, by overhead i meant overhead for the developer to write empty props but not on the performance. I was not pointing to the performance at all. What i meant was Eg i created 2 action creators with same function defn with props as 1st param and data as 2nd. And as a consistent pattern i followed it throughout

export const actionCreator1 = (props, data) => { .... }
export const actionCreator2 = (props, data) => { .... }

Consider actionCreator1 does something which doesn't need props at all while the actionCreator2 needs props but still if we have to follow same declaration pattern then we have to do the following while dispatching

dispatch(actionCreator1({}, data))
dispatch(actionCreator2({ someRequiredProp }, data))

Noticed that empty {} which was passed to actionCreator1 i was speaking about that overhead.
But instead if we use swap the position of the parameters in actionCreators and assign initial value of {} to props then we need not pass {} while dispatching actions which do not need props.
New version with parameter reversed

export const actionCreator1 = (data, props={}) => { .... }
export const actionCreator2 = (data, props={}) => { .... }

New dispatch calls, notice we didn't pass any props to actionCreator1 as they were not required

dispatch(actionCreator1(data))
dispatch(actionCreator2(data, { someRequiredProp }))

This might lead to a more consistent pattern which can be used throughout actionCreators i suppose.
We could also go with null instead of {} if you propose that is a better thing.
export const actionCreator = (data, props=null) => { .... }

UPDATE
@illuminist i think we might not be able to come up with a generic function definition because consider the below case of signOut actionCreator

export const signOut = firebase => {
	return (dispatch, getState) => {
		firebase
			.auth()
			.signOut()
			.then(() => {
				dispatch({ type: SIGNOUT_SUCCESS });
			})
			.catch(err => {
				dispatch({ type: SIGNIN_ERROR, err });
			});
	};
};

Here i just need the firebase instance to signOut the user i.e. i need to pass only props and no data, so in this case if i follow my suggested pattern i.e (data, props={}) i ended up doing,
export const signOut = (data, { firebase }) => { ... }
And then the call had to be
dispatch(signOut({}, { firebase })
So in that case i feel that we can't come up with a generic actionCreator function definition as some of them need either just data or just props or both data and props hence we can follow any convention which suits the use case. Also if you can come up with a generic pattern in mind please feel free to share with me as i will learn from it.

For 5. i just noticed they removed the deps array for useSelector. thank you for bringing it to my notice. Could i please request you to help me achieve this thing if we can't pass deps.
const todo = useSelector(state => state.todos[props.todo-id], [props.todo-id])
According to my understanding state.todos[props.todo-id], [props.todo-id] will give me an error stating wrong no of arguments right?

Is the below code a viable option

const todos = useSelector(state => state.todos)
const todo = todos[props.todo-id]

Or will this lead to major re-rendering issues?

[prescottprue]

Hey folks, just wanted to include a thought - we could possible use the action creators that already exist on the firebase instance by just passing dispatch, we would just need that instance.

Glad to see discussion on this!

[samsoul16]

@prescottprue One more thing, did you think about using useDispatch hook so that we don't need to initialize dispatch in rrfProps like dispatch : store.dispatch which we have to pass to ReactReduxFirebaseProvider. Is it actually possible? I'm referring specifically to this :

const rrfProps = {
	firebase,
	config: {
		userProfile: "users",
		useFirestoreForProfile: true,
		attachAuthIsReady: true
	},
	dispatch: store.dispatch,
	createFirestoreInstance
};

const Index = () => (
	<Provider store={store}>
		<ReactReduxFirebaseProvider {...rrfProps}>
			<App />
		</ReactReduxFirebaseProvider>
	</Provider>
);

Also any thoughts on the above discussion. I'm also eagerly waiting for @illuminist to reply. :)

[illuminist]

@samsoul16 I thought I replied yesterday before I getting back home, but turn out didn't and lost that reply ;(.

In my project, an action creator has single parameter to be whatever required to run an action. For example, createTodo({firebase, todoTitle, todoContent}). With this pattern, if an action doesn't require additional data I can create an action with just signOut({firebase}) to be used by dispatch. I think it's a bit sloppy but works well in my taste.

In your example, actually if a function doesn't use second argument, addition props in this case, you can just skip the second argument in function definition, export const actionCreator1 = (data) => { .... }. Combine with my pattern above, to have single argument for an action creator, the action creator definition will become:

export const createTodo = ({ firebase, todoTitle, todoContent }) =>  {
  return dispatch => { ... }
}

For useSelector case, to memoize a selector:

const todoSelector = useCallback(state => state.todos[props.todoId], [props.todoId])
const todo = useSelector(todoSelector)

This ensure the useSelector will have exactly same selector every time the component re-render until props.todoId changes.
Not sure why you use kebab-case for accessing object property which is invalid JS syntax. Better using camelCase as my example above.

According to my understanding state.todos[props.todo-id], [props.todo-id] will give me an error stating wrong no of arguments right?
JS doesn't throw an error on calling function with wrong number of argument
For example:

const add = (a, b) => {
  return a + b
}
console.log(add(2))

This doesn't throw an error. The function still return a result, which is NaN (from 2 plus undefined).
And it's also perfectly fine calling a function with more arguments than the function definition. Unless you're using typescript.

[samsoul16]

@illuminist That's a great proposal man. I love it and would go with this pattern now, where params can be anything, data, props, any extra thing i need. That's a great generic way to handle things, Love it :)

 export const actionCreator = (params) => {}

and then all the above use cases can be solved as follows

export const signIn = ({ credentials, firebase }) => { ... }
export const signOut = ({ firebase }) => { ... }
export const signUp = ({ newUser, firebase, firestore }) => { ... }

• Thank you for the memoize useSelector example, will use it exactly as stated.
• Thanks for the tip that we can call a function with less no of args then specified without resulting in an error.
• For the kebab case thing, actually I'm a clojure developer too hence while writing the Eg. i wrote in kebab-case, in actual code i use camelCase only.

[samsoul16]

Hello @illuminist can you please tell me how you solved this Warning/ Error
React Hook useCallback has a missing dependency: 'dispatch'. Either include it or remove the dependency array react-hooks/exhaustive-deps

const dispatch = useDispatch();
const signUpUser = useCallback(
	details => dispatch(signUp({ details, firebase, firestore })),
	[firebase, firestore]
);
const handleSubmit = e => {
	e.preventDefault();
	signUpUser(details);
};

the dispatch function is not a dependency there and it won't change with time so adding it to deps list doesn't make sense.
The solution i found on internet was adding this
// eslint-disable-next-line react-hooks/exhaustive-deps
before the function signUpUser but then we will have to do it in every file and every function in which we want dispatch, doesn't seem a good option.

[illuminist]

const signUpUser = useCallback(
	details => dispatch(signUp({ details, firebase, firestore })),
	[firebase, firestore, dispatch]
);

Neither firebase nor firestore instance change through time. The usage of dispatch is still in useCallback hook so it's counted as dependency of the hook. Eslint doesn't and maybe can't check that if any dependency variable will change or not so it reports missing dependency just to be safe.

[samsoul16]

Ah!! not the cleanest of things, again an overhead to pass it around, but for now i will be doing that, Thank you
Any ways it still works either ways, if you have it or you don't but those linting errors are a lot to watch.