prescottprue / redux-firestore

Redux bindings for Firestore
MIT License
575 stars 102 forks source link

dataReducer doesn't merge previous data on LISTENER_RESPONSE action. #294

Open unsouled opened 4 years ago

unsouled commented 4 years ago

What is the current behavior? dataReducer doesn't merge previous data on LISTENER_RESPONSE action.

What is the expected behavior? dataReducer should merge previous data on LISTENER_RESPONSE action when previous data is exist and meta does not have subcollections

Which version of redux-firestore are you using? What about other dependencies? 0.13.0

This commit(https://github.com/prescottprue/redux-firestore/commit/bab3aa5a2bd055897778a033d9b218df8f5478c9) breaks the action.

If intended, the above conditional code(https://github.com/prescottprue/redux-firestore/blob/d7a561361fecff128b35cf37712f852637a66f41/src/reducers/dataReducer.js#L53) is meaningless code.

prescottprue commented 4 years ago

Thanks for reporting. Feel free to make a PR if you get a chance. Otherwise, I'll get to it when I can

jake-nz commented 4 years ago

I think #218 means that the example given under Query Options for "or style queries" (with 'sally', 'john', 'peter') no longer works. I'm running into trouble trying to load different data from the same collection where the second query replaces the data from the first.

Maybe we could use something like mergeOrdered for mergeData instead?

giovannidavi commented 3 years ago

I have created a PR to revert the changes and add merged data back https://github.com/prescottprue/redux-firestore/pull/324

prescottprue commented 3 years ago

We might have to be careful here - merging logic was removed in the past since it caused an issue with params being removed from documents (from another client or server) after data has already been loaded on the current client. Maybe I'm misunderstanding, but just want to make sure we handle this case before moving forward since that test that was changed is in place for a reason

giovannidavi commented 3 years ago

Hey @prescottprue I was not aware of a related issue with the merge functionality, But is it intended to totally remove merging functionality to resolve that bug? is there any other way to keep the merge functionality while and resolve the removing issue?