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

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

Open
unsouled opened this issue Jul 9, 2020 · 5 comments
Open
Labels

Comments

@unsouled
Copy link

unsouled commented Jul 9, 2020

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(bab3aa5) breaks the action.

If intended, the above conditional code(

if (!previousData || meta.subcollections) {
) is meaningless code.

@prescottprue
Copy link
Owner

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
Copy link

jake-nz commented Aug 27, 2020

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
Copy link

I have created a PR to revert the changes and add merged data back
#324

@prescottprue
Copy link
Owner

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
Copy link

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants