Skip to content
This repository has been archived by the owner on Sep 10, 2022. It is now read-only.

reverse props spread order for mapPropsOnChange #102

Closed
hartzis opened this issue Feb 29, 2016 · 5 comments
Closed

reverse props spread order for mapPropsOnChange #102

hartzis opened this issue Feb 29, 2016 · 5 comments

Comments

@hartzis
Copy link
Contributor

hartzis commented Feb 29, 2016

While working with mapPropsOnChange recently we noticed that this.props get spread over computed props.

https://github.com/acdlite/recompose/blob/master/src/packages/recompose/mapPropsOnChange.js#L24

It appears the original plan was to spread computed props last?
#60 (comment)

Example:
I create an items prop with mapPropsOnChange

mapPropsOnChange(['thing'], ({thing}) => ({items: thing.count()}))

but if items was already a prop being passed down the new computed prop of items will get overwritten by this.props.items.

I would expect my outputted computed props to be what gets passed down.

Let me know what you think, and I could get a PR in with tests this week.

@acdlite
Copy link
Owner

acdlite commented Mar 1, 2016

Yes this was an oversight. It can be fixed by flipping lines 25 and 26. A PR would be great :)

@istarkov
Copy link
Contributor

istarkov commented Mar 1, 2016

For simplicity with line switch, omit on line 26 also can be removed. Otherwise it works like mapAndFilterPropsOnChange

@hartzis
Copy link
Contributor Author

hartzis commented Mar 1, 2016

@istarkov it appears mapAndFilterPropsOnChange is not a HOC in this library. I do like the idea of that though.

@istarkov
Copy link
Contributor

istarkov commented Mar 2, 2016

I meant that omit filtering was done because of merge order, if you switch lines 25 and 26, looks like there is no need to filter (omit) props taken from array.

@hartzis
Copy link
Contributor Author

hartzis commented Mar 2, 2016

I have the PR pretty much ready to go, but trying to run the tests local and they are currently failing with:
Module build failed: Error: The (relay-query) Babel 5 plugin is being run with Babel 6.

I think it is related to facebook/relay#889, and when they publish a new version it may be fixed.

I may just submit the PR and see if the travis build fails.

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

No branches or pull requests

3 participants