Skip to content

Feature/fix root collection serialization#27

Open
vedrani wants to merge 4 commits intorelease/v1from
feature/fix-root-collection-serialization
Open

Feature/fix root collection serialization#27
vedrani wants to merge 4 commits intorelease/v1from
feature/fix-root-collection-serialization

Conversation

@vedrani
Copy link
Contributor

@vedrani vedrani commented Jul 5, 2017

No description provided.

@vedrani vedrani requested a review from MrBr July 5, 2017 16:07

return _.reduce(state, (serializableState, substate, subStateKey) => {
const serializableSubState = transformSubstate(substate);
const transformedState = transformSubstate(state);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should change things a bit differently. It seems to me like transformSubstate should be the entry point for serialization and the reducer logic should be placed into helper which tranformSubstate uses.

It seems to me like we are processing data twice in some cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

transformXY is just transforming state (array to object), nothing else. Serialization is recursive process until you reach ordinary value. And in that process, you need to transform something or sanitize it (sanitizeElement ...). I think naming is wrong. When I look now and before, I think this two process are better if they are divided.

@vedrani vedrani requested a review from MrBr July 13, 2017 16:20
Copy link
Contributor

@MrBr MrBr left a comment

Choose a reason for hiding this comment

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

Looks good, good job with introducing traverse.

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.

2 participants