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

feat(dataReducer): revert merged data removal #324

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

giovannidavi
Copy link

@giovannidavi giovannidavi commented Feb 19, 2021

Description

Check List

If not relevant to pull request, check off as complete

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

Relevant Issues

#294

@codecov
Copy link

codecov bot commented Feb 19, 2021

Codecov Report

Merging #324 (5c8408f) into master (9622158) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #324      +/-   ##
==========================================
+ Coverage   89.98%   90.00%   +0.01%     
==========================================
  Files          19       19              
  Lines         629      630       +1     
==========================================
+ Hits          566      567       +1     
  Misses         63       63              
Impacted Files Coverage Δ
src/reducers/dataReducer.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9622158...5c8408f. Read the comment docs.

@@ -88,7 +88,7 @@ describe('dataReducer', () => {
`${collection}.${doc}.newData.field`,
data[doc].newData.field,
);
expect(result).to.not.have.nested.property(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was in place for a reason - not removing a value which is removed from the object will cause stale data in certain situations

@@ -59,11 +59,13 @@ export default function dataReducer(state = {}, action) {
state,
);
}
// Otherwise merge with existing data
const mergedData = assign(previousData, data);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will prevent removing a param from an object that is removed elsewhere - what is the goal here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR is meant to revert back an existing functionality that has been later removed (while kept in rrf - firebase), data merging.
When for a given collection you change the query to fetch new data (or next page for that matters) new data should be merged with existing data, at the moment is getting replaced.
What the PR is doing is just reverting back the files to their previous status (as mentioned in the issue open #294)

@prescottprue prescottprue changed the title Fix/revert merged data removal feat(dataReducer): revert merged data removal Jan 16, 2022
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

Successfully merging this pull request may close these issues.

None yet

2 participants